fix(github-pat): stop re.sub parsing the token as regex syntax (#2017)

[dolho]

Summary

_patch_env_github_pat passed the formatted .env line to re.sub as the replacement string, which re parses for escapes. A token carrying a backslash was read as a group reference:

ghp_a\g<1>b     -> re.error: invalid group reference 1 at position 20
ghp_back\slash  -> re.error: bad escape \s at position 20
ghp_with\1group -> re.error: invalid group reference 1 at position 21

Fixed with a callable replacement (lambda _match: new_line), which re inserts verbatim.

On severity — bounded, and I checked rather than assumed

_propagate_to_agent catches only httpx errors, so this escaped to the asyncio.gather(..., return_exceptions=True) in propagate_pat_to_all_agents and was recorded as that agent's failed with the message attached. The rotation continued for everyone else. So it fails honestly, not silently — which is why it's P3 and not higher.

What made it worth fixing anyway: the trigger is the token, not the agent, so it failed for every agent in the fleet at once, and told the operator bad escape \s — a message about regex syntax — rather than anything about the token they had just pasted.

Why not validate the PAT shape at the boundary

The issue offers that as the alternative, and I decided against it. GitHub ships at least six token formats (ghp_, gho_, ghs_, ghu_, ghr_, github_pat_, plus legacy 40-hex), and a regex tight enough to catch a stray backslash is tight enough to reject a format that ships next year. Rejecting a valid token is a worse failure than writing an invalid one through, and the AC's actual requirement is "never as re.error". Stating it because it's a judgement call, not an oversight.

The .env escaping half is split to #2023

Deliberately not bundled. It is not a PAT problem — GitHub PATs are [A-Za-z0-9_], so a quote can't appear in one — but it does affect every credential written through that path.

Fixing it means moving four things in one commit: the writer, the agent-side reader, credential_requirements_service._env_pairs (the ent#127 predicate is defined as byte-agreement with that parse), and the TestExporterParity fixtures that pin it — with the added constraint that _env_pairs's source is spliced into an in-container probe, so it must stay self-contained.

And it has to land on top of #2010, which rewrites that exact reader. Bundling it here would conflict directly with an open PR. #2023 carries the full analysis and the sequencing note.

Test plan

• tests/unit/test_2017_pat_line_escapes.py — 29 tests over seven hostile token shapes (group reference by name and number, unknown escape, trailing and doubled backslash, $, &), each on three axes: patching doesn't raise, the agent reads back the token exactly, and the re.sub path agrees with the append path
• The read-back axis matters independently: a replacement string could mangle \g<1> into a captured group without raising, silently writing a different token
• All three Wire the agent GitHub PAT for the gh CLI + REST API (not just git) #1574 key mirrors must agree — the first key takes the re.sub path and the other two are appended, so a replacement bug shows up as the keys diverging
• A static pin that the replacement is a callable. Every behavioural test above still passes for tokens containing no backslash — which is every real GitHub PAT — so only the call shape expresses the property being protected
• Mutation-verified: restoring line_re.sub(new_line, ...) fails 13 of the 29
• 1109 adjacent tests green (-k "pat or 1967 or 1574 or 1264 or propagation or credential")

Closes #2017

[obasilakis]

The fix is correct and the diagnosis is exact — re.sub parses a replacement string for escapes, a callable replacement is inserted verbatim, and a credential is precisely the kind of value that must never be parsed. 29 tests pass on the branch. The hostile-token table is well chosen (\g<1>, \1, \s, trailing backslash, $, &), and asserting through the agent's own .env reader rather than through the file contents is the right oracle.

Blocking on one thing: the test that pins the mechanism does not pin the mechanism. It is the only thing standing between this fix and a silent revert, and it passes with the fix reverted.

Blocking — test_the_replacement_is_a_callable_not_a_string is fail-open

src = inspect.getsource(mod._patch_env_github_pat)
assert "lambda" in src and ".sub(" in src, (
    "the .env line is being passed to re.sub as a replacement STRING again "
    "— a backslash in the token is then read as regex syntax (#2017)"
)

The docstring is right about why this test needs to exist:

A future edit could re-introduce line_re.sub(new_line, ...) and every behavioural test above would still pass for tokens that happen to contain no backslash — which is every real GitHub PAT.

But inspect.getsource returns the comments too, and this PR's own comment block opens with:

            # `lambda _: new_line`, NOT the string itself (#2017). `re.sub`

So "lambda" in src is satisfied by the prose regardless of what the code does. I reverted the fix on this branch — keeping the comment, as any real revert or bad conflict resolution would — and ran it:

ACTIVE CODE : out = line_re.sub(new_line, out, count=1)
BEHAVIOUR   : *** BUG IS BACK *** error: invalid group reference 1 at position 20
GUARD says  : PASS (green)
'lambda' found only in : ['# `lambda _: new_line`, NOT the string itself (#2017). ']

The bug is fully restored, the behavioural tests above it are all still green (none of them run under a reverted implementation — they only exercise the fixed path), and the one test written to catch this reports success.

This matters more than usual here because of #2025. Both PRs rewrite the same three lines, and whichever merges second has to hand-resolve a conflict where the wrong resolution reintroduces exactly this bug. That resolution is the moment this test is supposed to earn its keep.

Suggested fix — the sibling in #2025 already does it right. test_the_substitution_is_not_capped_at_one walks the AST and inspects the .sub() call rather than the text, and its docstring explains why it had to:

It has to read the CALL, 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.

Same trap, same function, same PR batch — caught in one and missed in the other. The equivalent here is: find the .sub() call, assert args[0] is not an ast.Constant/ast.JoinedStr (i.e. the replacement is not a literal string). I mutation-tested #2025's version both ways (count=1 keyword and positional third argument) and it went red on both, so the pattern is proven in this codebase.

This is now the fourth instance of the same class in docs/memory/learnings.md — a check that reads narrower than what it protects and therefore reports safe. Worth an entry.

Composition with #2025 — please read these together

They conflict, and neither side of the conflict is a correct resolution:

<<<<<<< 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

The function needs both properties. The correct composed line is:

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

Taking "ours" keeps count=1 and silently reopens #2016 — the worse direction, because that bug is itself silent: the rotation reports the agent as updated while the agent keeps authenticating with the revoked token. Taking "theirs" drops the callable and reopens #2017, which at least fails loudly.

I verified they do compose: with the line above, all of #2024's, #2025's and #2018's PAT tests behave as expected on the merged tree.

Whoever merges second should also delete the corresponding xfail(strict=True) marker from tests/unit/test_pat_propagation_properties.py (#2018) — for this PR that is test_a_backslash_in_the_token_does_not_raise, which otherwise turns the suite red by passing.

Confirmed clean

• Scope: service + its test + registry entry. Nothing extra.
• The agent_reads helper is byte-faithful to docker/base-image/agent_server/routers/credentials.py:379-386 — checked line by line.
• The append branch is untouched and the tests assert the two branches agree on hostile tokens, which is the right consistency check given only one of them goes through re.sub.
• No credential values in the diff or the test data.

[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.