Repository navigation
Stop silently launching unauthenticated workspaces #115
Description
Question
Stop silently launching unauthenticated workspaces.
Graduated from the credentials ticket,
which found something worse than the prototype that prompted it. Two halves of one
defect: the repo teaches an instruction that breaks auth, and the breakage is
invisible.
The instruction is wrong, and it is wrong on the host too
CLAUDE.md (a symlink to AGENTS.md:44-52) and dev.sh:11-16,128-130 teach:
XDG_CACHE_HOME=/tmp/dl-scratch/cache XDG_CONFIG_HOME=/tmp/dl-scratch/config dl-next owner/repo
The XDG_CONFIG_HOME half guards nothing. devlaunch stores nothing under it:
the only consumer is worktree/config.py:87-89 get_config_path(), whose sole
caller load_config() (:95-99) reads; grep -rn get_config_path finds no
writer. And the one thing devlaunch does write under a config dir —
completion.py:29-33 → ~/.config/devlaunch/completions.sh — hardcodes
Path.home() and ignores the variable entirely. XDG_CACHE_HOME does all the
real guarding (dl.py:79-86, worktree/storage.py:29-30, worktree/config.py:11-16).
So the scratch config dir has exactly two effects, both bad: it kills gh, and it
hides the user's real config.toml, making the scratch run less faithful than
an unscoped one.
And this is not a container-only problem. Verified read-only on the host:
XDG_CONFIG_HOME=/tmp/… gh auth token → rc=1, no oauth token found; unset →
rc=0. GH_TOKEN/GITHUB_TOKEN are unset in a normal host shell, so
resolve_token() falls through HOST_TOKEN_VARS straight to the broken gh.
Every dl-next run this repo has taught has been launching unauthenticated
workspaces. The container only made it visible.
Fix: delete the XDG_CONFIG_HOME half, in prose and in both command lines.
Keep XDG_CACHE_HOME. Deleting beats caveating (principle 2) — "except inside the
container" is a rule people get wrong.
The failure is unreachable, not merely quiet
Worse than "logs at debug": dl.py:75 pins basicConfig(level=INFO) with no env
override and no CLI flag, so gh_auth.py:77,80,84,115,121 can never be seen by
any user. The only tell today is an absence — --send-env GH_TOKEN missing
from dl.py:1010's INFO line — and the devpod up path has no tell at all.
Promote those to warnings. Warn on: gh rc≠0; gh raised or timed out;
non-token stdout; staging failure (both sites). Stay silent on:
forwarding_disabled(), and gh not on PATH — those are choices, not failures.
The rc≠0 message must include the effective XDG_CONFIG_HOME. Without it the
message tells a logged-in user to run gh auth login, which in exactly this case
is actively misleading. lru_cache on resolve_token already caps this at one
warning per process.
No type change. The resolving ticket checked: Optional[str] here is an honest
absence, not a sentinel — _is_token() parses at the boundary, and neither caller
branches on the reason; both do if not token: forward nothing. The illegal state
is behavioural, so principle 3 is satisfied and principle 2 picks the smaller fix.
This is a wayfinder:build ticket: go through /tdd.
Tests (the resolving ticket sketched these; test/unit/test_gh_auth.py's
autouse fixture at :15-23 already undoes test/conftest.py:41-46's suite-wide
logging opt-out, so caplog works):
- warns on rc≠0, and the config dir appears in the message text;
- does not warn when
ghis absent fromPATH; - does not warn and does not spawn when forwarding is opted out;
- junk stdout warns — and the junk never appears in any log record (this one is
a credential-safety test: a malformed token must not be echoed); - two calls produce one warning (pins the
lru_cache); - extend the existing
mkstempOSErrortest at:141-146.
Also one README sentence (~README.md:180) — the repo's README-sync rule applies.
Do not add a --verbose flag here. That five-site gap is recorded separately;
this ticket must not depend on it.
Activity
blooop commentedon Aug 8, 2026
breadcrumb: claimed and started. Building both halves as one change on
wayfinder/devlaunch-115.Seams, unchanged from what the ticket names — no new ones invented:
devpod upflag context manager, observed through emitted log records viacaplog, with theghsubprocess and the PATH lookup mocked at the process boundary (exactly where the existing unit tests already mock). No internal function is mocked, so renaming anything inside the module breaks nothing.Test-visible behaviour I intend to pin: a warning when the CLI exits non-zero (naming the effective config dir), when it raises or times out, when its stdout is not a token, and when staging the token file fails; silence when forwarding is opted out or the CLI is simply absent; one warning per process, since the resolver is cached. The credential-safety one asserts the malformed value is absent from every emitted record, not merely that a warning happened.
Fake token-shaped literals only in fixtures; no real credential value goes anywhere near this branch.
blooop commentedon Aug 8, 2026
breadcrumb: the visibility half is done and green — every failure path the ticket named now warns, and the two it named as choices rather than failures stay silent.
Two judgement calls worth recording:
The credential-safety test uses a junk value that is deliberately not token-shaped and checks its absence across the rendered message, the format string, the record arguments and the record's own attribute dictionary — not just the formatted output — so a future change that passes the value through as a log argument fails the test even if no handler renders it.
blooop commentedon Aug 8, 2026
breadcrumb: both halves landed on a branch; PR #119 is open against main.
The instruction half is verified the only way that settles it — on metal, read-only, on the host. With the config dir scoped the way the repo used to teach, the resolver now warns and forwards nothing; in a plain shell it forwards a token silently. That is the defect reproduced and the fix demonstrated in the same pair of runs, and it confirms the reach: this was never container-specific.
Full non-e2e suite green (954), formatter and both linters clean. No credential value appears in the diff, in any test fixture, in any log line the change emits, or in anything posted here — every literal is an obviously-fake placeholder.
Deliberately not done: no verbosity flag, so the four remaining unreachable debug sites elsewhere are untouched and that gap stays where it is recorded. Next stage is review in fresh context; this ticket stays open.
blooop commentedon Aug 8, 2026
breadcrumb: build stage complete → in review. Gate checked from GitHub state: PR #119, head
6b270f4b8e861b164825555ddb1000122f6fdb86, workflow runs present for that SHA and all checks green. Routing to/reviewin fresh context.blooop commentedon Aug 8, 2026
breadcrumb: review done in fresh context → needs attention. PR #119 reviewed on both axes at
efcaf2d...6b270f4; verdict request changes (small) — posted as a comment because GitHub refuses--request-changeson an own-authored PR.Spec: fully met. Both premises independently confirmed (
get_config_path()has no writer;completion.pyhardcodesPath.home()), all three doc sites done, rc≠0 warning reproduced on metal naming the effective config dir, both deliberate silences proved by mutation rather than inspection, no--verbose, no type change, no credential value anywhere.One blocking Standards item, one sentence to fix: the replacement prose in
AGENTS.md:55-57/dev.sh:16-18says scopingXDG_CONFIG_HOME"guards nothing". The deletion is right but that reason is false —config.tomlcan pinrepos_dirto an absolute path that beatsXDG_CACHE_HOMEoutright (worktree/config.py:78), demonstrated on metal, andtest/conftest.py:47-49already records the opposite conclusion for the same mechanism. This spec shares the overstatement, so it is worth correcting here rather than inheriting it.Credential-safety mutation results: the junk-stdout test earns its four layers — a lazy log arg is caught by
getMessage(), and anextra={}smuggle is caught byrepr(vars(record))only, defeating bothgetMessage()andcaplog.text. But that rigor is uneven: interpolating the token into the mkstemp warning, or smuggling it viaextra=at the fdopen site, both pass 32/32. Recommended, not blocking.Not merged; ticket stays open.
blooop commentedon Aug 8, 2026
breadcrumb: review-gate retry pushed as
63c6ac5onwayfinder/devlaunch-115(PR #119); CI green on that head.AGENTS.mdanddev.shno longer say scopingXDG_CONFIG_HOME"guards nothing". They now name the trade: it would stop a personalconfig.tomlpinningrepos_dirback at the real cache, and is dropped anyway because it hides the host'sghlogin on every run. Reproduced therepos_diroverride on metal in scratch dirs with a stand-inHOME(real cache never touched).test/conftest.pygained one sentence saying why the suite scopes both — it has already opted out of gh forwarding, so it has nothing to lose.extra={"tok": token}at the fdopen site) now fail, as does a third (a debug-level leak beside the warning). Fake tokengho_secretonly.devlaunch/xdg.pyconfig_home(), used bygh_authandworktree/config.py;_gh_config_home()deleted. A red-first test showed the old spelling resolvedconfig.tomlrelative to the cwd whenXDG_CONFIG_HOMEis empty. The threeXDG_CACHE_HOMEcopies were deliberately left alone.Suite 956 passed / 21 e2e deselected; e2e not run (shared Docker daemon). Not merged, not closed.
blooop commentedon Aug 8, 2026
breadcrumb: verification pass on PR #119 head
63c6ac5— approve. All three findings re-checked independently, not read off the reply. (1) Therepos_dirprecedence is structural atworktree/config.py:80(_get_cache_base()is the default argument, soconfig.tomlbeatsXDG_CACHE_HOMEoutright) and both arms of the trade reproduce on metal in scratch dirs with a stand-inHOME:repos_direscapes the scratch cache with onlyXDG_CACHE_HOMEscoped, andgh auth tokenis rc=1 scoped / rc=0 unscoped. "guards nothing" is gone from the tree; the sole PR-body hit is a negation. (2) All three mutations re-run here — token in the mkstemp warning,extra={"tok": token}at fdopen, alogging.debugleak — each fails now, and the first two pass 32/32 against the pre-fix test file, confirming the "before" column. One extra: with the level assertion deleted, the debug leak is still caught byassert_reached_no_log_record, soat_level(DEBUG)is load-bearing on its own. (3)xdg.config_home()is byte-identical to the old spelling for unset and set — no live path moves for a normal user; onlyXDG_CONFIG_HOME=""changes, and the claimed cwd-relativePosixPath('devlaunch/config.toml')reproduces exactly. Suite 956 passed / 21 deselected; worktree restored clean at63c6ac5. Residual non-blocking:AGENTS.md:47's "everything … through XDG_CACHE_HOME" vscompletion.py:33's hardcoded write;dev.sh:16-21reads less clearly than the AGENTS.md sentence; and the threeXDG_CACHE_HOMEcopies are semantically identical, so that de-dup would move no path — cheaper than the reply implies. Nothing created or deleted on the host;devpod listunchanged by this pass.blooop commentedon Aug 8, 2026
breadcrumb: stage → in review, approved. PR #119 green at
63c6ac5, all three findings independently verified. Merging is yours; no steering line granted merge-when-green, so I am not merging it.The verification went past confirming the author's table. It re-derived the precedence claim structurally —
worktree/config.py:80puts_get_cache_base()in the default argument, so aconfig.tomlvalue beatsXDG_CACHE_HOMEoutright rather than incidentally — and it ran the "before" column itself rather than assuming it (both credential mutations applied against6b270f4pass 32/32, confirming they really were invisible). It then tested one thing nobody asked for: with the level assertion deleted, thelogging.debugleak still fails, at the absence check.at_level(DEBUG)is load-bearing on its own, not merely via the record count.Confirmed no live path moves for a normal user: old and new config-dir spellings are byte-identical for unset and set
XDG_CONFIG_HOME; only the empty case changes, and that one was resolving to a cwd-relativePosixPath('devlaunch/config.toml')— a real latent bug in a function that had no tests at all before this PR.