Summary
The autoreview secret masker (skills/autoreview/scripts/autoreview) rewrites the userinfo portion of ssh:// / URL string literals to the placeholder redacted, including cases that are not credentials: a bare user@host (no password) and a template-interpolation slot such as a Python f-string {var}@host. Because the masked text is what the review model reads, benign code that constructs SSH remote URLs from a variable is presented to the model as a hardcoded secret, and two textually-distinct constructions collapse into one identical literal.
This is the same over-masking class as the recently merged #129 (TypeScript secret references), #130 (Boolean declarations), and #131 (credential config paths) — all fixed in the last day — but for URL/SSH userinfo, which none of those three covers.
Reproduction
A test fixture that builds SSH remote URLs for two distinct principals and asserts they produce distinct identity digests:
digests = []
for ssh_user in ("alice", "bob"):
url = f"ssh://{ssh_user}@git.example.invalid/org/repo.git"
digests.append(remote_identity_digest(url))
assert digests[0] != digests[1] # distinct principals -> distinct identitiesObserved (text the review model receives after masking)
digests = []
for ssh_user in ("redacted", "bob"):
url = f"ssh://redacted@git.example.invalid/org/repo.git"
digests.append(remote_identity_digest(url))
assert digests[0] != digests[1]Two things were rewritten:
- The f-string URL's userinfo slot
{ssh_user} → redacted, on both loop iterations, so the URL literal is now identical each pass.
- The first tuple element (a bare username that also appears in userinfo position elsewhere in the file) →
redacted, while the second was left intact.
Expected
A bare user@host userinfo with no password component, and an interpolation slot like {var}@host, are not secrets and should not be masked. Only userinfo carrying an actual secret component (user:password@host) should be redacted.
Impact
The review model, reading the masked text, sees both loop iterations assign the same literal and reports the trailing assert digests[0] != digests[1] as a deterministic test failure — a false-positive finding on correct code that passes CI and local runs. In a downstream pipeline that runs this skill, the same phantom P1/P2 has recurred on every review cycle whose diff contains a userinfo-bearing fixture, each time costing a finding slot and a manual refutation. The redacted@ token in a quoted finding is itself the tell that the model saw masked, not real, source.
Likely location
The placeholder literal redacted is in SECRET_PLACEHOLDER_VALUES / URI_PASSWORD_PLACEHOLDER_VALUES, applied at the userinfo boundary that uri_authority_end() locates. There is already f-string-aware machinery here (brace_interpolation, the (?:f|fr|rf|…) prefix detection, and the (?<!@)\$ guard), which suggests a fix would plug into the same path — extend it so a userinfo with no password separator, and an interpolation-slot userinfo, are treated as non-secret.
Suggested direction
Restrict URL-userinfo masking to userinfo that contains a : password separator with a non-reference secret value; leave bare user@host and {interpolation}@host untouched. This mirrors the reference-vs-literal distinction #129/#131 already established for JS/TS config paths.
Happy to test a candidate fix against the fixtures that currently trip it if that's useful.
Summary
The autoreview secret masker (
skills/autoreview/scripts/autoreview) rewrites the userinfo portion ofssh:/// URL string literals to the placeholderredacted, including cases that are not credentials: a bareuser@host(no password) and a template-interpolation slot such as a Python f-string{var}@host. Because the masked text is what the review model reads, benign code that constructs SSH remote URLs from a variable is presented to the model as a hardcoded secret, and two textually-distinct constructions collapse into one identical literal.This is the same over-masking class as the recently merged #129 (TypeScript secret references), #130 (Boolean declarations), and #131 (credential config paths) — all fixed in the last day — but for URL/SSH userinfo, which none of those three covers.
Reproduction
A test fixture that builds SSH remote URLs for two distinct principals and asserts they produce distinct identity digests:
Observed (text the review model receives after masking)
Two things were rewritten:
{ssh_user}→redacted, on both loop iterations, so the URL literal is now identical each pass.redacted, while the second was left intact.Expected
A bare
user@hostuserinfo with no password component, and an interpolation slot like{var}@host, are not secrets and should not be masked. Only userinfo carrying an actual secret component (user:password@host) should be redacted.Impact
The review model, reading the masked text, sees both loop iterations assign the same literal and reports the trailing
assert digests[0] != digests[1]as a deterministic test failure — a false-positive finding on correct code that passes CI and local runs. In a downstream pipeline that runs this skill, the same phantom P1/P2 has recurred on every review cycle whose diff contains a userinfo-bearing fixture, each time costing a finding slot and a manual refutation. Theredacted@token in a quoted finding is itself the tell that the model saw masked, not real, source.Likely location
The placeholder literal
redactedis inSECRET_PLACEHOLDER_VALUES/URI_PASSWORD_PLACEHOLDER_VALUES, applied at the userinfo boundary thaturi_authority_end()locates. There is already f-string-aware machinery here (brace_interpolation, the(?:f|fr|rf|…)prefix detection, and the(?<!@)\$guard), which suggests a fix would plug into the same path — extend it so a userinfo with no password separator, and an interpolation-slot userinfo, are treated as non-secret.Suggested direction
Restrict URL-userinfo masking to userinfo that contains a
:password separator with a non-reference secret value; leave bareuser@hostand{interpolation}@hostuntouched. This mirrors the reference-vs-literal distinction #129/#131 already established for JS/TS config paths.Happy to test a candidate fix against the fixtures that currently trip it if that's useful.