Stop silently launching unauthenticated workspaces - #119
Conversation
Two halves of one defect: the repo taught an instruction that breaks gh auth, and the breakage was invisible. The instruction. The scratch-run recipe in the agent instructions and in dev.sh scoped XDG_CONFIG_HOME as well as XDG_CACHE_HOME. dl writes nothing under XDG_CONFIG_HOME -- its only consumer reads, and the one thing dl does write under a config dir hardcodes the home directory -- so that half guarded nothing while hiding the host's gh login from `gh auth token`. Deleted, with one clause on why; XDG_CACHE_HOME does all the real guarding and stays. The invisibility. logging is pinned at INFO with no override, so the five debug lines in gh_auth could never be seen by any user: a workspace opened logged out and said nothing. Those failure paths now warn -- a non-zero gh exit, gh raising or timing out, non-token stdout, and either staging failure. The non-zero-exit message names the effective XDG_CONFIG_HOME, without which it tells a logged-in user to run `gh auth login`, which in exactly this case fixes nothing. Not warned on: forwarding opted out, and gh not installed. Those are choices, not failures. No value is ever logged, including the malformed stdout case, and the test for it asserts absence across the rendered message, the format string, the record arguments and the record's attributes. Closes #115
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideMakes GitHub auth failures visible and fixes workspace launch instructions so they no longer hide the host’s gh login, while ensuring token values are never logged and failures don’t block workspace launches. Sequence diagram for GitHub token resolution and warning behaviorsequenceDiagram
participant dl
participant gh_auth
participant gh_cli as gh
participant logging
dl->>gh_auth: _token_from_gh_cli()
alt gh not in PATH (shutil.which)
gh_auth-->>dl: None
else gh in PATH
gh_auth->>gh_cli: subprocess.run(['gh','auth','token'])
alt OSError or SubprocessError
gh_auth->>logging: warning("Could not read a GitHub token from gh (%s)...")
gh_auth-->>dl: None
else non_zero_exit
gh_auth->>logging: warning("gh auth token exited %s...%s", returncode, _gh_config_home())
gh_auth-->>dl: None
else stdout not _is_token
gh_auth->>logging: warning("gh auth token printed something that is not a token...")
gh_auth-->>dl: None
else valid _is_token(stdout)
gh_auth-->>dl: token
end
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
+ Coverage 90.41% 90.68% +0.27%
==========================================
Files 17 18 +1
Lines 1982 1987 +5
==========================================
+ Hits 1792 1802 +10
+ Misses 190 185 -5
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed in fresh context against two independent axes, at the merge-base fixed point
efcaf2d...6b270f4. Preflight green: all CI (py310–py313), prek, GitGuardian and both
codecov checks pass. Claims were verified by reproduction and mutation, not read.
Standards
1. AGENTS.md and dev.sh now state a falsehood, in the file that teaches agents how not to destroy state — blocking (one-sentence reword).
AGENTS.md:55-57 and dev.sh:16-18 assert that scoping XDG_CONFIG_HOME "guards nothing —
dl writes nothing under it". The first clause is true; the inference is not. config.toml
can pin repos_dir to an absolute path, and it wins outright over XDG_CACHE_HOME:
# devlaunch/worktree/config.py:78
repos_dir=Path(worktree_data.get("repos_dir", _get_cache_base() / "repos")),Demonstrated on metal, scratch dirs only, with a config.toml containing
[worktree] repos_dir = "…/.cache/devlaunch/repos":
scratch XDG_CACHE_HOME only (what this PR now teaches):
repos_dir resolves to: /home/…/.cache/devlaunch/repos <-- the REAL one
both scoped (what this PR deleted):
repos_dir resolves to: …/scratchpad/demo/cache/devlaunch/repos
That is precisely the "costs someone their workspace list" hazard the same bullet warns
about two sentences earlier. The repo has this written down already, and it says the
opposite of the new prose — test/conftest.py:47-49:
"XDG_CONFIG_HOME goes with it: config.toml can point repos_dir back at the real cache,
which would defeat the isolation from the other direction."
The command lines are right to drop the variable — the gh breakage is real and worse.
What needs fixing is the reason given. Reword to the actual trade-off ("scoping it hides
your gh login; leaving it unscoped lets a personal config.toml redirect repos_dir")
instead of the absolute "guards nothing". Latent for anyone with no config.toml, which
is why it is cheap to fix and easy to trust by mistake.
2. _gh_config_home() is a second, divergent spelling of an existing concept — non-blocking.
gh_auth.py:69 is os.environ.get("XDG_CONFIG_HOME") or str(Path.home()/".config");
worktree/config.py:89 is Path(os.environ.get("XDG_CONFIG_HOME", Path.home()/".config")).
They disagree on XDG_CONFIG_HOME="": the new or form treats empty as unset (correct per
XDG), the older form does not. The new one is the better spelling — the smell is that it is
the second copy. The new from pathlib import Path is justified; gh_auth.py had no prior
Path use.
3. Message overclaims what was consulted — non-blocking.
gh_auth.py:62 says "the config directory gh just consulted" and :98 prints "gh read its
config from %s", but gh reads $XDG_CONFIG_HOME/gh and honours GH_CONFIG_DIR ahead of
it. The named directory is still the actionable one; the phrasing is just stronger than the code.
Checked and clean. Root-logger logging.<level> is the house convention (43 call sites
vs 3 getLogger(__name__), all in worktree/) — the PR matches it. The five repeated
"…without a GitHub login." clauses are a deliberate and correct call: whole sentences at
their own sites, and log strings are a poor DRY target. No test asserts on message wording,
so rewording won't break them. No ordering dependence: the autouse fixture
(test/unit/test_gh_auth.py:15-23) cache_clear()s both sides of the yield; 32 passed
across repeat runs, and ::TestFailuresAreVisible passes alone. fail_but_do_not_leak_the_fd
correctly closes the fd before raising. dev.sh:12 is an under-filled reflow (58 chars vs
74–81 for neighbours) — cosmetic.
Spec
Every acceptance criterion in #115 is met. No blocking Spec findings.
Half A's premise independently confirmed, not taken on faith. The spec says
"grep -rn get_config_path finds no writer" — confirmed: one caller, load_config()
(worktree/config.py:93), which tomli.loads and returns; save_config survives only as a
CHANGELOG.md:222 reference to a removed function. The spec says "completion.py:29-33 …
hardcodes Path.home()" — confirmed at completion.py:28-33, with only a
DEVLAUNCH_COMPLETION_FILE override. The premise holds, so the deletion is right.
All three sites done (AGENTS.md:47-56, dev.sh:12-19, dev.sh:131-132). A whole-repo grep
for XDG_CONFIG_HOME leaves no site still teaching the bad recipe: the remaining hits are the
new don't-do-this prose, container paths pointing at the real /home/vscode/.config, test
scaffolding, and readers.
"The rc≠0 message must include the effective XDG_CONFIG_HOME" — reproduced on metal,
read-only, real gh, no gh state touched, boolean-only token check:
WARNING:root:gh auth token exited 1, so this workspace opens without a GitHub login.
gh read its config from …/demo/ghscratch -- if you are logged in on this host, that
directory is the thing to check before `gh auth login`.
token forwarded: False
(unscoped, ordinary host shell) token forwarded: True
Emitted at the basicConfig(level=INFO) dl.py:75 actually sets, so it reaches a real user —
which the five logging.debug sites it replaces never could. That is the defect and the fix
in one pair of runs.
Five warn sites present (gh_auth.py:89,95,107,141,151): raise/timeout, rc≠0, non-token
stdout, and both staging sites. The two deliberate silences are asserted, not merely
absent — proved by mutation in an isolated copy, not by inspection: adding a
logging.warning to the shutil.which branch fails test_no_gh_installed_says_nothing, and
adding one to the forwarding_disabled() branch fails
test_opting_out_says_nothing_and_asks_gh_nothing. Both go green again on restore (32/32).
All six enumerated tests exist and test what was asked. The seventh (fdopen write failure)
is in scope, not creep: the spec requires "staging failure (both sites)", and the
enumerated list would have left the second site shipping unexercised. Calling that out rather
than quietly padding the count is the right instinct — as is saying plainly that three tests
passed on arrival.
"Do not add a --verbose flag" — none added. "No type change" — Optional[str]
unchanged; no evidence found against that ruling, and nothing here warrants relitigating it.
Spec note, non-blocking. The spec's own "XDG_CACHE_HOME does all the real guarding" is
the same overstatement flagged under Standards, and the spec went further: it argued that
hiding config.toml makes a scratch run "less faithful". test/conftest.py:47-49 records
the opposite conclusion for the same mechanism. The PR implemented the spec faithfully; the
spec is what was incomplete.
Credential safety: clean, and stress-tested. No real credential value appears anywhere in
the diff, fixtures or emitted log lines — the only literals are gho_secret and a junk value
deliberately not token-shaped. I mutated the credential-safety test to confirm it earns its keep:
| mutation at the junk-stdout site | result |
|---|---|
value passed as a lazy log arg — warning("…: %s", stdout) |
caught by record.getMessage() |
value smuggled via extra={"gh_stdout": …} |
caught by repr(vars(record)) only — invisible to getMessage() and caplog.text |
The four-layer assertion is not belt-and-braces padding; the second case defeats every
single-layer check. This test is the strongest thing in the PR.
But that rigor is not applied evenly — non-blocking, and the one thing I'd add. The two
staging sites have a real token in scope, and their assertions are weaker. Two more
mutations, both of which the full suite passes 32/32:
| mutation | result |
|---|---|
interpolate token into the mkstemp failure warning |
not caught — that test asserts levelno only |
smuggle token via extra={"tok": token} at the fdopen site |
not caught — that test asserts caplog.text only |
The spec only demanded credential-safety on the junk test, so this is not a spec miss. But the
PR description claims "no token value is logged on any path", and today nothing holds that
line at the two sites where a token actually exists. Lifting the junk test's four-layer check
into a shared helper and using it at all three sites would make the claim true by test.
Verdict
Request changes — small and surgical. The engineering is genuinely good: the premise was
verified rather than asserted, the failure is reproduced and fixed in the same run, the
deliberate silences are pinned by tests that actually fail when broken, and the
credential-safety test survives a mutation that defeats the obvious version of itself.
One blocking item, fixable in one sentence:
- Standards #1 — reword the
AGENTS.md:55-57/dev.sh:16-18justification. Keep the
deletion; drop "guards nothing" for the real trade-off. As written it contradicts
test/conftest.py:47-49and understates a workspace-list hazard in the file agents read
to avoid exactly that.
Recommended, not blocking: the shared credential-absence assertion at the two staging sites
(Spec, final section), and the _gh_config_home() duplication (Standards #2).
No finding contradicts a decision recorded on the map. The type-change ruling stands.
The scratch-run prose justified the deletion with "guards nothing -- dl writes nothing under it". The first clause is true and the inference is not: a config.toml under XDG_CONFIG_HOME can pin repos_dir at an absolute path that beats XDG_CACHE_HOME outright, which is the blast radius the recipe exists to contain, and test/conftest.py already said so. The deletion still stands -- the gh breakage is unconditional while the repos_dir hazard needs a config.toml most hosts do not have -- so AGENTS.md and dev.sh now name it as that trade instead, and conftest.py says why the suite makes the opposite call (it has already opted out of gh forwarding). Also: extend the four-layer "no credential reached a log record" assertion to the two staging sites, where a real token is in scope, via one shared helper; both now capture at DEBUG so a leak below WARNING is caught too. And give XDG_CONFIG_HOME one spelling in devlaunch/xdg.py, which fixes the older reader resolving config.toml relative to the cwd when the variable is exported empty.
All three findings addressed in 1 (blocking) — the prose now states the trade, not the falsehoodYou were right, and the demonstration reproduces. Re-run here in scratch directories with a stand-in
One addition beyond the reword: The PR description carried the same overstatement in its own words; it has been rewritten 2 (should-fix) — one helper, three sites, verified by mutationBoth weak assertions were exactly where you said. The four-layer check is now Your two mutations, plus a third for the level change — each run against the full file, each
The description's "no token value is logged on any path" is now backed rather than asserted. 3 (non-blocking) — unified, in the direction you called correctBoth readers now go through a new def config_home() -> Path:
return Path(os.environ.get("XDG_CONFIG_HOME") or Path.home() / ".config")
Deliberately not moved: the three copies of the Not redoneEverything you confirmed as met — spec coverage, both asserted silences, the README Suite: 956 passed (954 + the two new config-path tests), 21 e2e deselected. The e2e suite was |
Verification pass on 1 (was blocking) — verifiedThe prose is now true clause by clause, and I checked the load-bearing one myself rather than repos_dir=Path(worktree_data.get("repos_dir", _get_cache_base() / "repos")),
The other half of the trade too, read-only on this host, exit codes only, no So both arms the new wording weighs against each other are real, and the asymmetry it asserts Two wording nits, neither false, neither blocking:
2 — verified by my own mutations, one each, run against the full file and reverted
Both of the first two, applied together against the pre-fix test file ( One extra mutation the reply did not claim, because the third row above stops at the level 3 — verified; no live path moves for a normal user
The claimed cwd-relative On leaving the three New surface and description
PR description: "guards nothing" replaced with the trade and the demonstration; "no token Nothing new introduced. Everything the previous pass confirmed as met is untouched.
VerdictApprove. All three findings genuinely addressed, and finding 2 came back stronger than |
# Conflicts: # CHANGELOG.md
1 participant
Closes #115.
Two halves of one defect: the repo taught an instruction that breaks
ghauth, and the breakage was invisible.This has been mis-instructing users on the host, not only in the container
The scratch-run recipe this repo teaches — in the agent instructions and in
dev.sh, twice — scopedXDG_CONFIG_HOMEalongsideXDG_CACHE_HOME. Doing so hides the host'sghlogin fromgh auth token, so the token forwarded into the workspace is no token at all.That is not a container-only effect. Verified read-only on this host, before and after the change:
GH_TOKEN/GITHUB_TOKENare unset in an ordinary host shell, so the resolver falls straight through to theghthe recipe just broke. Every run made the documented way has been opening a workspace with no GitHub credentials, on the host as much as inside a container — the container only made it noticeable.Dropping the variable is a trade, not a free removal. It costs a real guard: a personal
config.tomlunderXDG_CONFIG_HOMEcan pinrepos_dirat an absolute path that beatsXDG_CACHE_HOMEoutright (worktree/config.py), which is exactly the blast radius the recipe exists to contain, andtest/conftest.pyscopes both for that reason. Demonstrated on metal in scratch directories, with a stand-inHOMEso the real~/.cache/devlaunchwas never read or written:The deletion still stands, because the two harms are not the same size: the
ghbreakage happens on every run made the documented way, while therepos_dirhazard needs aconfig.tomlmost hosts (including this one) do not have. SoAGENTS.mdanddev.shname it as that trade rather than claiming the variable guards nothing, andtest/conftest.pynow says why the suite makes the opposite call — it has already given up gh forwarding, so it has nothing to lose by scoping both.The failure was unreachable, not merely quiet
Logging is pinned at
INFOwith no environment override and no CLI flag, so the five debug lines in the auth module could never be seen by any user. The only tell was an absence — a missing flag in one INFO line — and thedevpod uppath had no tell at all.Now warned on: a non-zero
ghexit,ghraising or timing out, stdout that is not a token, and either failure to stage the token file. Deliberately still silent: forwarding opted out via the escape hatch, andghnot installed — those are choices, not failures.The non-zero-exit message names the effective
XDG_CONFIG_HOME. Without it, the message tells a logged-in user to rungh auth login, which in exactly this case fixes nothing. The resolver's cache caps this at one warning per process.Credential handling
No token value reaches a log record on any path — and every site with a token or a maybe-credential in scope now asserts that the same way, through one helper checking four layers: the rendered message, the format string, the record arguments, and the record's own attributes. Four, because no single layer holds: a lazily interpolated
%sargument renders nothing intorecord.msg, and anything smuggled throughextra=is invisible to bothgetMessage()andcaplog.text. The two staging tests also capture atDEBUGnow, so a leak logged below the warning is caught while the level assertion still pins exactly oneWARNING. Verified by mutation — a token interpolated into the mkstemp warning, a token smuggled viaextra=at the fdopen site, and a debug-level leak beside the warning each fail the suite now and passed it before. Every literal in the tests is an obviously-fake placeholder.One spelling for the config home
XDG_CONFIG_HOMEwas read in two places that disagreed: the new warning treated an empty value as unset (correct per the XDG basedir spec), whileworktree/config.pydid not, and so resolvedconfig.tomlrelative to the working directory. Both now go throughdevlaunch/xdg.py, so the directory the warning names is the one the loader actually reads. The three copies of theXDG_CACHE_HOMElookup (dl.py,worktree/storage.py,worktree/config.py) are the obvious next tenant of that module, but moving cache paths around is not this change's business.Not here
No
--verboseflag. That gap is recorded separately and this change does not depend on it. No type change either: the resolving investigation established that the optional return is an honest absence rather than a sentinel, and neither caller branches on the reason.Tests
Six behaviours pinned at the existing seam (the token resolver and the
devpod upflag context manager, with the subprocess and the PATH lookup mocked at the process boundary): warns and names the config dir on a non-zero exit; names the home-relative default when the variable is unset; warns whenghhangs; warns on junk stdout without repeating it; stays silent whenghis absent; stays silent and spawns nothing when opted out; warns once however often a run asks. Both staging sites are covered — the existing temp-dir test extended, and a new one for the write failure that the ticket's list would otherwise have left unexercised. Plus two on whereconfig.tomlis looked for, one of which was red beforedevlaunch/xdg.pyexisted.Full non-e2e suite: 956 passed. Format, ruff, ty, pylint and prek clean.
🤖 Generated with Claude Code
Summary by Sourcery
Ensure GitHub CLI credential forwarding failures are surfaced and avoid misconfigurations that silently open unauthenticated workspaces.
Bug Fixes:
Enhancements:
Documentation:
Tests: