fix(deploy): read a quoted key_env, and test the direction that can hurt you

[OsherElhadad]

Follow-up to #92, which was merged before its review landed. The review found that one of
its two fixes introduced a false PASS — strictly worse than the false FAIL it replaced.
The review of this PR then found that the same false-PASS class was still open in three
more shapes, so the extraction below is the second iteration and this time it was checked
against the parser rather than against a fixture.

The defect

#92 rewrote the credential extraction as:

sed -nE 's/#.*//; s/.*key_env:[[:space:]]*([A-Z0-9_]+).*/\1/p'

[[:space:]]* requires the variable name to begin immediately after the whitespace, so a
quoted value is not extracted at all:

key_env: "UPSTREAM_QUOTED_KEY"
key_env: 'UPSTREAM_SINGLE_KEY'

Both forms are legal — config/upstreams.go:30 is a plain KeyEnv string and the loader
parses them — and config/upstreams.go:93 refuses to boot when a named variable is
unset. So the sequence was: preflight prints Preflight PASSED, the operator proceeds,
and the service then fails to start on precisely the credential preflight existed to check.
Under Restart=always that is a crash loop.

A false PASS in a credential check is worse than the false FAIL it replaced.

The fix

A regex over config is only right or wrong relative to what the parser does, so the
check is a differential one: an oracle links config.LoadUpstreams and prints the
key_env names it would demand, and the extraction is diffed against it over 41 inputs
covering every shape the loader accepts. A name in truth but not in the extraction is a
false PASS; the reverse is only an annoying false FAIL.

Three independent causes had to be closed, none of them the quoting the first attempt
addressed:

shape	before	now
key_env: "X" / key_env: 'X'	missed	✓
two or more key_env on one physical line (flow style)	only the last	✓ all of them
a # inside a quoted scalar earlier on the line	erased the real key_env	✓
"key_env": X — a quoted mapping key	missed	✓
key_env: >- with the name on the next line (block scalar)	missed	still missed, knowingly

So: comments are stripped the way YAML defines them ((^|[[:space:]])#) rather than at any
#; the line is then split on the flow separator so "one name per line" stops mattering;
and the colon no longer has to touch key_env. The order is load-bearing in both
directions and a test pins it: strip after splitting and the tail of a comma-bearing
comment comes back as configuration.

A block scalar stays unhandled and the in-code comment now says so rather than claiming
generality. Nobody folds an environment variable name, and closing it needs a real YAML
parser — which is the recorded upgrade path, explicitly in preference to a fifth sed
stage. The two other unhandled shapes (a lowercase or mixed-case name) at least fail
loud: they extract noise that matches no credential file and print ✗.

Every stage was mutated and the test re-run: reverting to the previous extraction, dropping
the split, un-YAML-ing the comment strip, requiring the colon to touch key_env, stripping
comments after the split, stripping none, and over-stripping (/#/d) each turn a test
red.

The test gap that let it ship

TestPreflightResolvesToolsOnTheServicePATH asserted only that a present tool is
found — the direction that cannot hurt you. A test that never exercises the failing
direction cannot catch a false PASS, which is exactly how this shipped.

It now asserts all four states:

caller-tool-is-runnable-here=yes    the plant exists where the caller can see it
on-service-path=found
caller-only=missing                 on the caller's PATH, absent from the service PATH
nowhere=missing

What the first assertion guards is a vacuous pass, not a broken tool. Typo the planted
filename and the test goes red; remove the first assertion and typo the plant and it goes
green — so the line is load-bearing. It cannot detect a tool that is present but
unrunnable, because command -v — the primitive in_service_path uses — reports a
mode-0600 file as found (measured unprivileged; the same file cannot be executed).
That case is outside what this assertion can see, and the in-code comment says so.

Also closed, from the same review

A ui/ subdirectory silently disabled ALL asset versioning. fs.Glob(fsys, "*")
returns a subdirectory as a name, fs.ReadFile on it errors, and the whole function then
bails to "no assets" — so one nested file would turn versioning off everywhere and bring
back the stale-asset bug #92 existed to remove, silently and one level down. Replaced with
fs.WalkDir, which covers assets in subdirectories at their slash-separated names — which
is also what a reference to one looks like in the markup.

The remaining ceiling on the rewrite is named in a ponytail: comment rather than papered
over: it matches quoted asset names blindly, so a quoted occurrence that is not a URL (a JS
string literal, a CSP nonce, inside a percent-encoded data URI) would be rewritten too.
There is no collision across the five assets today — four rewrites, all real references.

The upgrade path is NOT to narrow the match, and the comment now says why. Matching on
an enclosing HTML attribute would be a regression: one of the four references is a
JavaScript object literal, el('link', {href: 'tools.css'}) in dash/ui/tools.js, not an
attribute. An attribute match would stop versioning tools.css, hand it back an
unversioned URL at max-age=3600, and reintroduce the stale-stylesheet bug the
one-token-for-all-assets design exists to prevent. The blindness is load-bearing precisely
because references live in both an attribute and an object literal.

What lands instead makes the failure loud rather than the match narrower: three lines
totalling the rewrites actually performed and comparing them against the references the
test already enumerates. Planting a quoted asset name that is not a reference now turns
that assertion — and only that assertion — red; remove the assertion and the same plant is
silent again.

Two comment corrections in the same area, both cases of a comment claiming more than the
code or the test delivers:

• The asset hash writes the file name and length. Both new sub-cases in the test are
  decided by the NUL-delimited name alone, so the comment no longer implies they cover the
  length. The length stays, with its real reason recorded where it is written: it is what
  makes the concatenation injective, and without it {"a": "b.js\0xy"} and
  {"a": "", "b.js": "xy"} hash identically.
• The PATH test's illustration cited /sbin and /bin being absent from systemd's default
  PATH. On a usrmerge distribution those are symlinks into /usr, which systemd's PATH
  does contain, so the example produced no discrepancy. Replaced with the real one already
  documented in install.sh: /usr/local/bin is in systemd's PATH and not in sudo's
  secure_path. The test's mechanism was right; only the prose was wrong.

Verification — both directions, on the real host

sudo ./deploy/service/install.sh preflight → Preflight PASSED against the deployed
allow-list, unchanged from before this PR, and the extraction still yields nothing at rc 0
on the shipped upstreams.example.yaml — the no-regression case, since the shipped file's
documenting comment is the false FAIL #92 set out to fix.

The other direction, against a temporary config that configures a quoted key_env and a
flow-mapped one on a single line, with no credential files present: both names are demanded
and preflight prints Preflight FAILED with a ✗ for each. Nothing live was modified,
restarted or reconfigured.

CGO_ENABLED=1 go test -race -count=1 ./... → 25 packages ok, exit 0. go vet clean.
gofmt -l . empty. No schema change; schemaVersion untouched at 6.

[OsherElhadad]

Review — 03f1b5e

APPROVE-WITH-NITS. Both halves of the claim hold. The reported false PASS is closed,
and — the part that matters more — the guard test now fails in the direction that can hurt
you: the append-instead-of-override mutation that stayed green against #92 is now
red. The WalkDir swap is correct and measurably behaviour-preserving. The ponytail:
no-collision claim is checkable, and it is true.

One item I would want landed (R1): the false-PASS class is not closed, only the reported
instance — and the new comment tells the next reader the function fails safe, which is
true for the case it names and false for three others. Remedy is two lines plus two fixture
lines, supplied below. Everything else is a nit.

No dollar, token or latency figure is claimed, so there is nothing to re-derive — the right
choice for this change, and worth saying out loud.

How F1 was checked

A regex over config is only right or wrong relative to what the parser does, so rather
than eyeball shapes I built an oracle: a program linking the real config.LoadUpstreams
that prints the key_env names it would demand, diffed against the shipped extraction over
the same file. 41 inputs. A name in truth but not in sed is a false PASS; the reverse is
merely annoying.

The four shapes in the PR body are correct, and so are eleven nobody had tried: tab after
the colon, key_env: with no value, key_env: "", a digit-leading name, an
underscore-leading name, CRLF, a YAML anchor and its alias, !!str, a ---
multi-document header, a 65 KB line, no trailing newline, and a single flow-mapped entry.
Four inputs the loader itself rejects produce noise, which cannot matter — the service will
not boot on them either.

Five diverge.

---

R1 — the false-PASS class is not closed, and the new comment says it is

deploy/service/install.sh:70-82

The fix makes the extraction independent of quoting. It does not make it independent of
line structure. Measured, loader-vs-preflight:

input	loader demands	preflight extracts
upstreams: [{…, key_env: UPSTREAM_A_KEY}, {…, key_env: UPSTREAM_B_KEY}, {…, key_env: UPSTREAM_C_KEY}]	A, B, C	C only
upstreams: [{name: 'a#b', …, key_env: UPSTREAM_A_KEY}]	A	nothing
"key_env": UPSTREAM_A_KEY	A	nothing
key_env: >-⏎ UPSTREAM_A_KEY	A	nothing

Three root causes, none of them the quoting this PR fixed:

1. One name per line. s///p has no g and the leading .* is greedy, so on a line
   with several key_env: only the last is taken.
2. s/#.*// is not YAML-aware. A # inside a quoted scalar earlier on the line is not
   a comment, but the substitution treats it as one and erases the real key_env: after it.
3. The colon must touch key_env. "key_env": — a quoted mapping key — never matches.

Each ends the way the PR body describes: Preflight PASSED, then LoadUpstreams refuses to
boot, and Restart=always makes it the crash loop install.sh's own header (lines 17-20)
exists to prevent.

The fail-safe claim about a lowercase name is true, and I checked it end to end rather
than by reading: key_env: upstream_a_key extracts _, which the credential mapping
(install.sh:334-341) turns into $CREDS/- and $CREDS/_, neither of which exists, so
preflight prints ✗ and returns 1. Same for the U and KEY that mixed-case and flow-style
noise produce. That direction is safe as advertised.

Exposure today is nil, which is why this is a nit and not a block: the shipped example is
block style, the deployed allow-list is block style, and no doc invites flow style. Note too
that the plausible inline style — one flow-mapped entry per line, - {name: a, …, key_env: UPSTREAM_A_KEY} — extracts correctly, because there is still one key_env per line. It
takes two entries on one physical line to break it.

But the body says "No special case per quoting style, and nothing to update when a third one
appears," and install.sh:76-79 tells the next reader that being wrong here lands in the
safe direction. For the four rows above it lands in the unsafe one, and a wrong safety claim
in a comment is what stops the next person from checking.

Fix — 2 lines, differentially tested over all 41 inputs, closes all four rows, regresses
nothing (the shipped example and the deployed allow-list both still yield nothing at rc 0):

configured_key_envs() {
  sed -E 's/(^|[[:space:]])#.*//' "$1" 2>/dev/null | tr ',{}' '\n\n\n' \
    | sed -nE 's/.*key_env[^:A-Z0-9_]*:[^A-Z0-9_]*([A-Z0-9_]+).*/\1/p' | sort -u
}

• (^|[[:space:]])# is what YAML actually calls a comment, so 'a#b' survives while a real
  trailing comment still dies. Stripping before the tr keeps a comma inside a comment
  from resurrecting the tail of it.
• tr ',{}' puts each flow-mapped entry on its own line, so "only the last per line" stops
  mattering.
• key_env[^:A-Z0-9_]*: accepts a quoted key. Still no quote character in the sed
  script, so the '\'' escaping you avoided stays avoided.

Two fixture lines guard it, in the same test:

  - {name: flow, dialect: openai, base_url: 'https://x#y', key_env: UPSTREAM_FLOW_A_KEY, header: X-Api-Key}
    "key_env": UPSTREAM_QUOTED_NAME_KEY

Block scalars stay broken and are worth leaving broken — nobody writes a folded scalar for an
env var name, and closing it needs a real parser. Say so in the comment instead of claiming
generality. If you would rather not touch the code, narrowing those two claims is enough to
make this a documentation nit.

R2 — nit: the body overclaims what caller-tool-is-runnable-here=yes proves

The body says that without it, caller-only=missing "could pass because the tool was simply
broken rather than because the resolution is correct." Broken is the one case this assertion
cannot see — bash's command -v, the primitive in_service_path itself uses, reports a
non-executable file as found:

$ chmod 600 $d/probe600; PATH="$d:$PATH" bash -c 'command -v probe600 && echo FOUND'
/…/probe600
FOUND                      # unprivileged, mode 0600

and planting the caller-only tool at 0o600 instead of 0o700 accordingly leaves the test
green.

What it does guard is a vacuous pass, and it guards that properly: typo the planted
filename and the test goes red; remove the assertion and typo the plant and it goes green.
The line is load-bearing. The in-code comment at preflight_test.go:143-144 already says
this correctly ("a typo in the tool name") — only the body's wording drifts.

R3 — nit: the new hash test proves the NAME is in the hash, not the LENGTH

dash/assetversion_test.go:135-152, dash/ui.go:93-96

mutation	test
drop %s\x00%d\x00 entirely	red
keep the length, drop the name	red
keep the name, drop the length	green
keep both, drop the \x00 delimiters	green

Both new sub-cases are decided by the NUL-delimited name alone — in {a.js:"xy", b.js:""}
vs {a.js:"x", b.js:"y"} the delimiter already lands in a different place. So "the name
and the length … go into the hash" is guarded at half strength.

Keep the length — it is what makes the concatenation injective, and it is reachable to
defeat without it: {"a": "b.js\x00xy"} and {"a": "", "b.js": "xy"} hash identically once
%d is gone. One clause in the test's comment, or a third pair differing only in a length.

R4 — nit: the walk closure aborts the walk, so one bad entry still means "no assets"

dash/ui.go:73-82

Reading it closely, as it deserves: it is correct as written, on three counts. The
err != nil check precedes d.IsDir(), so the nil DirEntry that comes with a walk error is
never dereferenced. It is unreachable in production — fsys is fs.Sub(embed.FS), every
byte is in the binary, and ReadDir on an existing embedded directory has nothing to fail at.
And aborting is the better choice anyway: swallowing the error would hash a partial asset
set and mint a token that does not describe what is served, where falling back to unversioned
is the fail-open the doc comment promises. Mutating the closure to return nil leaves both
dash tests green, so the path is untested — the right amount of testing for a branch that
cannot fire.

R5 — nit: the new comment's illustrative PATH example does not hold on a usrmerge distro

cmd/context-guru-proxy/preflight_test.go:123-126

"sudo's secure_path carries /sbin and /bin, systemd's default PATH does not." Literally
true, but /sbin and /bin are symlinks into /usr on any usrmerge distro, and systemd's
default PATH contains /usr/sbin and /usr/bin — so a tool "in /sbin" is reachable by the
unit after all, and that example produces no discrepancy. The real one is the reverse,
already documented at install.sh:56-63: /usr/local/bin is in systemd's PATH and not in
sudo's secure_path. The test's mechanism is right — it plants a directory on the caller's
PATH and not the service's — only the prose picked the wrong illustration.

---

The blind-rewrite ceiling

The no-collision claim is true, by two independent measurements. Enumerating every quoted
occurrence of every asset name across all five assets, with the same loose bracketing the code
uses (["']name["'], mismatched quotes included), yields exactly four hits — and instrumenting
the real versionedUI counts exactly four rewrites:

index.html: 3 rewritten reference(s)
tools.js:   1 rewritten reference(s)
TOTAL = 4

All four are real references: index.html:7 href="style.css", :742 src="app.js", :745
src="tools.js", tools.js:35 href: 'tools.css'.

Recommendation: do not land the attribute match — and change the recorded upgrade path,
because as written it is a regression. The fourth reference is dash/ui/tools.js:35:

document.head.appendChild(el('link', { rel: 'stylesheet', href: 'tools.css' }));

A JavaScript object literal, not an HTML attribute. "Match on the enclosing attribute" would
stop matching it, tools.css would go back to an unversioned URL at max-age=3600, and that
is exactly the stale-stylesheet bug the one-token-for-all-assets design exists to prevent —
named in this same file at ui.go:43-46. The blindness is load-bearing because references
live in both an attribute and an object literal. Tightening the match is not a strictly-safer
upgrade; it trades a hypothetical silent over-rewrite for a concrete silent under-rewrite of a
reference that exists today.

What should land instead, if anything: make the failure loud rather than the match
narrower. The worry is sound — a silent miss in the response path of every page load is the
shape this whole thread started with — but the cheap fix is a test, not a regex. The rewrite
count is knowable and stable at 4, and the test already enumerates the references it expects.
Three lines asserting that the total number of ?v=+assetVersion occurrences across
versionedUI equals that enumeration turns any future collision into a red test instead of a
silent rewrite, at zero runtime cost. So: keep the blind rewrite, keep the ceiling named
(naming it is sufficient for the code), replace "Match on the enclosing attribute if one
ever appears" with the reason it is not an option, and optionally add the count assertion —
which is the guard the ceiling actually wants.

---

Do the new tests guard their properties?

Mutation is the only thing that establishes that a guard test guards anything. Every fix was
mutated and the tests re-run, -count=1 throughout.

mutation	test
revert to [[:space:]]* (the #92 defect)	red
strip whole-comment-lines only (F4's gap)	red — F4 closed
no comment stripping at all	red
over-strip: /#/d	red
accept lowercase names too	green — not a claimed property; uppercase-only is documented
append the service PATH instead of overriding	red — was green against #92
bare command -v (pre-#92)	red
hardcode systemd's default PATH	red
typo the planted caller-only tool	red (vacuous-pass guard works)
plant the caller-only tool 0o600	green — R2
drop callerBin from the test PATH	red
revert fs.WalkDir to fs.Glob(fsys, "*")	red — F8 closed
WalkDir but SkipDir on every subdirectory	red
walk closure swallows the error	green — R4
drop name+length from the hash	red
drop the name, keep the length	red
drop the length, keep the name	green — R3
drop the \x00 delimiters	green — R3
ServeContent(w, r, "", …)	red — F6's new assertion guards
rewritable = map[string]bool{}	red

Both headline gaps from the #92 review are shut, and shut in the direction that matters.

F4–F7 and P2

• F4 — closed. The # key_env: UPSTREAM_NEVER_CONFIGURED fixture plus the exact compare
  makes whole-comment-lines-only stripping red.
• F5 — taken, half-guarded. See R3; keep the length.
• F6 — taken, and beyond the ask. The dead block and the mime import are gone and a
  Content-Type assertion was added. path is still needed for path.Ext; vet and build clean.
• F7 — taken. The claim inside the comment verifies; the upgrade path inside it does not.
• P2 — rejected, and I agree. in_service_path exists to resolve a tool the way the
  unit will; a tool the caller can see and the unit cannot must read as missing, or you get
  the same false PASS in a different check. Appending reinstates exactly that, and the new
  caller-only=missing assertion now locks the override in — I re-ran the append mutation and
  it is red. The source is right too: no unit sets Environment=PATH=, so
  systemctl show-environment is the manager environment the units inherit rather than an
  approximation, and the ${p:-$PATH} fallback preserves the old behaviour where systemctl
  is absent. Only quibble: the rejection is recorded implicitly, as a test, not in the body —
  and a test is the better record, so I would not change it.

Verified sound

• F8 is behaviour-preserving. assetVersion is unchanged from a33c9ac, so swapping
  Glob for WalkDir over today's flat asset directory changes no URL and busts no cache. The
  new subdirectory case versions correctly: sub/extra.js is hashed at its slash-separated
  name and rewritten to "sub/extra.js?v=…", which is also the name the handler looks up, so a
  nested asset is served from versionedUI rather than falling through. WalkDir skips the
  . root, so no directory enters names; lookup is an exact map hit, so nested keys add no
  traversal surface. Untouched edge, not this PR's: a nested sub/index.html would get
  max-age=3600 rather than no-cache, since switch name compares the whole path.
• No regression on the real inputs. The shipped example and the deployed allow-list both
  extract nothing at rc 0, unchanged. This also re-confirms #92's F3: the deployed allow-list
  configures no key_env, so Preflight PASSED there exercises the zero-configured path — the
  right evidence for the reported bug, and not evidence for the quoted case, which the unit
  test now covers instead.
• Conventions. DCO sign-off present; author and committer are both the repo identity;
  conventional-commit fix(deploy): title written as a human would. No AI attribution
  anywhere in the diff, commit message or body. No secret, key, token, UUID, username or home
  path in the added lines. schemaVersion still 6 at dash/schema.go:43, untouched.
• Scope is clean. Four files, every hunk in service of something the body claims, no
  unrelated refactor, nothing left dead.

Commands

gofmt -l .                                   # empty
go vet ./...                                 # exit 0
CGO_ENABLED=1 go test -race -count=1 ./...   # 25 packages ok, exit 0
                                             # both #91 flakes green in this run

Plus the differential oracle (a main package with a replace onto the branch, linking
config.LoadUpstreams) over 41 inputs; a credential-mapping replay of install.sh:334-341
against the extracted noise; the mutation matrix above, each applied to a throwaway copy and
reverted; a quoted-occurrence audit of dash/ui/* plus a throwaway dash test counting ?v=
in the real versionedUI. Nothing was pushed to the branch and no service was touched.