Prepare repo for open-source release - #108
Conversation
Addresses the open-source readiness audit in #107. Add Apache-2.0 LICENSE. The repo previously had no license, which meant publishing it would not have made it open source — default copyright would apply and nobody could use, modify, or redistribute it. Remove the hardcoded repo1.dso.mil default host. A default host makes misconfiguration silent: a user whose config failed to load still got a working-looking glk pointed at an instance they never chose. It also weakened the pinned-host safety invariant, which is only meaningful when the operator picked the pin. The host is now required and validated at the two request chokepoints (Client.Get, Client.getStream) rather than at startup, so offline commands — init, config, doctor, version, install-skills, install-hooks — keep working. That ordering matters: doctor is what you run to diagnose a missing host. grec checkout gained the same guard; without it, an unconfigured checkout built a hostless clone URL (https:///group/project.git) and handed it to git. doctor now reports a missing host as a problem rather than printing an empty value with "(source: default)". Remove gitlab-kiosk-prd.md and docs-local/ — internal product and ops notes framed around a specific engagement. ARCHITECTURE.md already carries the design rationale a contributor needs, and now documents why there is no default host. Fix packaging for unauthenticated users: the Homebrew formula and Makefile tap URL used SSH, which only worked for someone with push access. Both use HTTPS now, the formula declares its license, and the README drops the GOPRIVATE instructions and a hardcoded personal checkout path. Point the work-items search help at docs.gitlab.com instead of a host-specific /help URL, and neutralize the skill file's framing. Crow-Session: E3CF07BB-A433-4ABA-9A23-2A3FB3B61671 Co-Authored-By: Claude <noreply@anthropic.com>
dhilgaertner
left a comment
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Solid, well-argued PR. The license is correct, the host-default removal is the right call, and the ADR reasoning about chokepoint-vs-startup validation is genuinely good — glk doctor must not require the thing it diagnoses. Two things need fixing before merge.
Critical Issues
None. Nothing here is a security regression, and the safety invariants (GET-only, public-only, pinned host) are intact or strengthened.
🟡 1. Client.GraphQL is a third request path, and it is unguarded
internal/gitlab/client.go:324
The PR body and the new ADR both state that host validation happens at "the two request chokepoints (Client.Get and Client.getStream)". There are three. Client.GraphQL builds its own url.URL at client.go:338 and calls c.HTTP.Do at client.go:355 without ever passing through Get.
glkcli/epic_notes.go:83 calls it with client.Host directly. When the group argument is a path rather than a numeric ID — the common case — fetchEpicNotes skips the GetGroupPath lookup entirely and goes straight to GraphQL, so nothing guarded runs first.
Verified end-to-end against make build:
$ GLK_CONFIG=/nonexistent/config.toml GITLAB_HOST= ./glk epic notes big-bang 123
executing request: Get "https:///api/graphql?query=query+workItemNotesByIid%28%24fullPath%3A+ID%21%2C
+%24iid%3A+String%21%2C+%24pageSize%3A+Int%2C+%24after%3A+String%2C+%24sort%3A+WorkItemDiscussionsSort
%29+%7B%0A++namespace%28fullPath%3A+%24fullPath%29+%7B%0A++++id%0A++++workItem%28iid%3A+%24iid%29+%7B%
0A++++++id%0A++++++widgets%28onlyTypes%3A+%5BNOTES%5D%29+%7B%0A ... [~1.2 KB elided] ...
": http: no Host in request URL
exit=1
Compare the guarded paths, which are exactly right:
$ ./glk project big-bang/bigbang
no GitLab host configured — set gitlab.host in your config file (run `glk init`) or set GITLAB_HOST
$ ./glk work-items big-bang --search foo
fetching group: no GitLab host configured — set gitlab.host in your config file (run `glk init`) or set GITLAB_HOST
This is the same bug class the PR correctly fixed in grec checkout — a hostless URL (https:///…) escaping into a lower layer — just missed in the one path that doesn't go through Get. Not a security hole (the request fails rather than reaching a wrong host), but it dumps a 1.2 KB URL-encoded query at the operator instead of the actionable error, and it makes the ADR's architectural claim inaccurate on the day it lands. In a repo where CLAUDE.md makes ARCHITECTURE.md binding, that matters.
Fix is three lines mirroring the existing guards:
func (c *Client) GraphQL(host, query string, variables map[string]interface{}) ([]byte, error) {
if host == "" {
return nil, ErrNoHost
}Worth a TestGraphQL_EmptyHostReturnsErrNoHost alongside the two new chokepoint tests, and updating the ADR to say "three request chokepoints (Client.Get, Client.getStream, and Client.GraphQL)".
🟡 2. Stale host-resolution claim left in ARCHITECTURE.md
ARCHITECTURE.md:83
Host resolution:
GITLAB_HOSTenv var >config.tomlhost > defaultrepo1.dso.mil.
This survived the edit and now contradicts two other places in the same file: the updated Pinned host invariant at line 64 ("There is no default host") and the new ADR at line 187 ("It is no longer a default"). The PR caught the stale "hardcoded in code" claim a few lines below; this one is the same category and was missed.
Suggested: Host resolution: GITLAB_HOST env var > config.toml host. There is no default — see "Why there is no default GitLab host".
🟢 3. glk init scaffolds a functional host the operator didn't pick
glkcli/init.go:13 writes host = "gitlab.com". The ADR's argument is that "pinning is only meaningful when the operator chose the pin," and a scaffold that emits a real, working host is a soft version of that.
Genuinely weaker than the original problem, and I don't think it blocks: the value lands in a plaintext file the user ran a command to create, it's annotated # Required…there is no default, and doctor/config show both report it with source: config gitlab.host. The silent failure mode is gone either way — a config that fails to load now yields an empty host and a loud error. Consider host = "gitlab.example.com" so a forgotten edit fails fast rather than querying a real instance; reasonable to leave as-is if you'd rather glk init produce something immediately usable.
🟢 4. artifactArchiveRedirectHosts remains deployment-specific
glkcli/artifacts.go:25-29 still hardcodes repo1.dso.mil → its S3 bucket. Correct to keep — it's a fail-closed redirect allowlist, and an empty host simply misses the lookup — but it's the one remaining Platform One coupling in a repo about to go public, and operators on other instances get no artifact-archive redirects. A comment noting it's an allowlist rather than a default would age well.
Security Review
Strengths:
- Removing the default host genuinely strengthens the pinned-host invariant, and the ADR argues it correctly. Ambient defaults make "pinned" a claim about the tool rather than about operator intent.
- Chokepoint validation over startup validation is the right ordering.
doctoris what you run to diagnose a missing host, so gating it on one would be circular.glk doctor,config show,init, andversionall still work unconfigured — confirmed. doctornow fails closed (glkcli/doctor.go:492-497): missing host iserror+problem: true, so the command exits non-zero. Verified nothing in.github/,Makefile, orlefthook.ymldepends onglk doctorexiting 0, so no CI breakage.- LICENSE is canonical Apache-2.0 — 202 lines, appendix boilerplate correctly instantiated with
Copyright 2026 Radius Method, LLC.Formula/gitlab-kiosk.rb:5declares it and the README states it. - Homebrew ssh→https keeps its integrity pin.
Formula/gitlab-kiosk.rb:4-6retainsrevision: d6c74a7…alongside the tag, so unauthenticated install doesn't trade away commit pinning. - GET-only and public-only are untouched. No new HTTP method, no new credential header, no change to the visibility check or the known-safe allowlist.
- Independent spot-check for credential patterns (
glpat-,ghp_, AWS key IDs) across non-test, non-fixture files: only the# "gitlab.com" = "glpat-..."comment ininit.go:18. Consistent with the PR's gitleaks/TruffleHog result; I did not re-run full-history scans.
Concerns:
- Finding 1 above is the only one with a security-adjacent flavor, and it's containment rather than exposure: the hostless GraphQL URL fails at the transport instead of being refused at the chokepoint. No request leaves for an unintended host.
Code Quality
- Test coverage for the change is good and targeted:
ErrNoHostfrom both guarded chokepoints, message actionability asserted againstgitlab.host/glk init/GITLAB_HOST,grec checkoutrefusal including the--hostmention, anddoctorreporting the missing host. The gap is the third chokepoint, not the rigor. TestRunDoctor_Healthywriting a real config file is the right adjustment — "healthy" legitimately means something different now.TestLocate_DefaultHostInfersConfig→TestLocate_ConfiguredHostDisambiguatesis a good rename; the old name described a mechanism that no longer exists.hostSource := "unset"atglkcli/doctor.go:472is now unreachable when the host is non-empty (with no default, a set host implies env or config). Harmless and arguably correct as a defensive fallback — noting it only so it isn't mistaken for a live branch.
Verification I ran (not taking the PR body's word for it):
| Check | Result |
|---|---|
go vet ./... |
clean |
Full suite via signed path (go test -exec shell/go-test-codesign-exec.sh ./...) |
all 13 packages pass |
make check-skills |
clean, no skill drift |
make build |
succeeds, both binaries signed |
| Binary behavior, unconfigured host | project ✅ · work-items ✅ · doctor ✅ · epic notes ❌ (finding 1) |
Summary Table
| Color | Meaning | Verdict effect |
|---|---|---|
| Red | Must fix | Request changes |
| Yellow | Should fix | Request changes |
| Green | Consider | Approve allowed |
Recommendation: Request Changes — driven by 0 Red, 2 Yellow, 2 Green findings.
Finding 1 is the substantive one; finding 2 is a one-line doc correction. Both are small, and the PR is otherwise in good shape — this should be a quick round trip. Agreed on holding the visibility flip as a separate, human-owned step.
Client.GraphQL builds its own url.URL and calls c.HTTP.Do directly rather than delegating to Get, so it was not covered by the ErrNoHost guards added in the previous commit. glk epic notes reaches it directly whenever the group argument is a path rather than a numeric ID — the common case — because fetchEpicNotes skips the GetGroupPath lookup that would otherwise have been guarded. Unconfigured, that produced a ~1.2 KB URL-encoded transport error (https:///api/graphql?query=...) instead of the actionable message. Same bug class as the hostless grec checkout clone URL, missed in the one path that does not route through Get. Not an exposure — the request fails rather than reaching an unintended host — but it made the ADR's architectural claim inaccurate on the day it landed. The ADR now says three chokepoints, names GraphQL's reason for needing its own guard, and states the rule for any future method that builds its own request. Also fixes a stale claim the previous commit missed: ARCHITECTURE.md still documented host resolution as falling back to "default repo1.dso.mil", contradicting both the updated Pinned host invariant and the new ADR in the same file. A sweep for other default-host claims found none remaining. glk init now scaffolds gitlab.example.com rather than gitlab.com. A scaffold that emits a real, working host is a soft version of the problem the ADR argues against — an operator who runs init and forgets to edit would query an instance they never chose. The placeholder does not resolve, so an unedited config fails immediately and names the host to fix. Document artifactArchiveRedirectHosts as a fail-closed allowlist rather than a default, since it is the one remaining deployment-specific map. Crow-Session: E3CF07BB-A433-4ABA-9A23-2A3FB3B61671 Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — both yellow findings were real, and I verified each against the built binary rather than taking them on faith. All four addressed in 9a9ba30. 1. 🟡
|
dhilgaertner
left a comment
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Strong PR. The licensing, disclosure, and packaging work is correct and the chokepoint-vs-startup reasoning in the new ADR is the right call. I verified the three request chokepoints are genuinely exhaustive — rg for url.URL{ in internal/gitlab/ returns exactly lines 122 (Get), 198 (getStream), and 340 (GraphQL), all now guarded, plus line 247 which is the already-validated redirect inside getStream.
One blocking issue: the guard audit covered every path that builds a request, but not the other consumers of client.Host.
Critical Issues
🔴 glk hook run blocks all unrelated traffic when no host is configured — glkcli/hook.go:58,70
strings.Contains(s, "") is true for every s. Before this PR client.Host was never empty (it defaulted to repo1.dso.mil), so both call sites were always meaningful. Removing the default makes host == "" the reachable default state, and the hook turns into a blanket deny:
hook.go:58—strings.Contains(url, host)matches every WebFetch URL.hook.go:70—strings.Contains(command, host)matches every command, so the token scan runs and returnstruefor any command whose first word iscurlorglab, regardless of target.
Confirmed empirically against this branch (temporary test in glkcli, since testSetup always sets a non-empty Host and no existing test covers the empty case):
=== RUN TestReviewProbe_HookEmptyHost_WebFetch
BUG CONFIRMED: unrelated WebFetch blocked with message "Use glk for access. Run 'glk --help' for usage."
=== RUN TestReviewProbe_HookEmptyHost_BashCurl
BUG CONFIRMED: unrelated curl blocked with message "Use glk for access. Run 'glk --help' for usage."
Failure scenario: a new user installs glk, runs glk install-hooks, and hasn't yet run glk init. Every WebFetch and every curl/glab invocation in their Claude Code session is rejected with exit 2 and a message containing an empty host — Use glk for access. The hook is installed in ~/.claude/settings.json and persists, so this is sticky and the message gives no clue what's wrong.
This is the same silent-misconfiguration failure mode the PR set out to eliminate, relocated from the API client into the hook. Suggested fix, matching the file's existing fail-open convention (it already fails open on unreadable and malformed stdin):
host := client.Host
if host == "" {
return nil // fail open: nothing to protect without a configured host
}Worth broadening the ADR's closing invariant too. It currently says "Any future method that constructs its own url.URL instead of delegating to Get must add the same guard" — the rule that would have caught this is about every consumer of the host value, not just request builders.
Code Quality
🟡 Auto-detected project path gives the unhelpful error — glkcli/project_resolve.go:18,37
resolveProject calls gitremote.DetectProject(client.Host) before any request is made. With an empty host, remoteHost == host never matches, so the user gets the wrong diagnosis. Verified against a build of this branch, inside a git repo with a repo1.dso.mil remote and no config file:
$ glk issues
no project argument provided and auto-detection failed: no git remote matching host "" — provide a project argument explicitly
$ glk issues big-bang/bigbang
fetching project: no GitLab host configured — set gitlab.host in your config file (run `glk init`) or set GITLAB_HOST
The explicit-project form is exactly right. The auto-detect form is the one agents hit most often (running inside a checkout is the documented workflow), so the confusing message is the first one most users will see. An ErrNoHost check in resolveProject/resolveProjectArgs — or in DetectProject itself — closes the gap.
🟡 No coverage for the empty-host state outside the client
The new tests cover ErrNoHost from all three chokepoints, grec checkout, and doctor — good. But glkcli.testSetup unconditionally assigns a non-empty Host, so the state this PR creates is untested in glkcli, which is why both issues above slipped through. A test asserting the hook is a no-op with no configured host would have caught the Red finding directly.
🟢 Dead default in glkcli/doctor.go:472 — hostSource := "unset" is no longer reachable as displayed output: when the host is empty the detail is replaced wholesale by the hint, and when non-empty the source is always GITLAB_HOST or config gitlab.host.
🟢 glkcli/work_items.go:13 — https://docs.gitlab.com/ee/user/search/advanced_search.html works, but GitLab has since dropped the /ee/ prefix and the .html suffix from canonical doc URLs. Worth confirming and using the current shape so it doesn't rely on a redirect.
Security Review
Strengths:
- The pinned-host argument is correct, not just tidier. An implicit default meant a failed config load still produced a working-looking client aimed at an instance the operator never chose. Requiring the host makes the invariant honest.
- Chokepoint placement is right, and the
GraphQLguard is a real catch —glk epic notesreaches it directly whenever the group argument is a path rather than a numeric ID (epic_notes.go:83,104), bypassing theGetGroupPathlookup at line 69. Guarding onlyGet/getStreamwould have left that path open. grec checkoutfix prevents a hostless URL reachinggit(https:///group/project.git) — a genuine find, and the error correctly mentions the--hostflag.artifactArchiveRedirectHostsfail-closed comment is accurate.artifactArchiveRedirectHosts[""]yields a nil map, so an unconfigured host follows no redirects, andgetStreamrejects it before that matters. Documenting it as an allowlist rather than a default is the right framing.glk initplaceholder is well chosen.gitlab.example.comis RFC 2606 reserved and does not resolve, so an unedited config fails fast rather than reaching a real instance.- Secret-scan methodology is sound — two independent scanners over full history (not the audit's shallow clone), with the one gitleaks hit correctly identified as entropy on the string literal
keycloak-0.txt. No history rewrite needed, agreed. - No credential or token handling changed.
tokenresolution, redaction (redactPresignedURL), and the GET-only structure are untouched.
Concerns:
- The hook regression above is a safety-control availability issue rather than an exposure one: it fails closed on unrelated traffic, so nothing leaks. But a security control that blocks everything is one users disable, which is how it becomes an exposure issue.
Verification I ran
| Check | Result |
|---|---|
go vet ./... |
clean |
Full suite via signed test path (-exec shell/go-test-codesign-exec.sh) |
all packages ok |
make check-skills |
clean — generated skill files current |
LICENSE |
canonical Apache-2.0, 202 lines, Copyright 2026 Radius Method, LLC |
Chokepoint audit (rg 'url.URL{' internal/gitlab/) |
3 construction sites, all guarded |
| Empty-host hook probe | 2 failures — bug confirmed |
| Empty-host auto-detect probe (built binary) | confusing error confirmed |
Temporary probe test was removed; the working tree is unchanged.
Summary Table
| Color | Meaning | Verdict effect |
|---|---|---|
| Red | Must fix | Request changes |
| Yellow | Should fix | Request changes |
| Green | Consider | Approve allowed |
Recommendation: Request Changes — driven by 1 Red, 2 Yellow, 2 Green findings.
The Red is a small fix (a three-line early return) and the Yellows are close behind it. Everything else — the license, the ADR, the disclosure cleanup, the packaging changes, the secret scan — is ready to ship.
The LICENSE copyright line named "Radius Method, LLC"; the company is Radius Method, Inc. A copyright notice naming an entity that does not exist is a defect in the one file whose whole job is to be legally correct. Verified the corrected LICENSE still differs from the canonical Apache-2.0 text by exactly one line — the copyright line — so license detection, go-licenses, and SPDX scanners still recognize it. Surface the warranty disclaimer in the README. Apache-2.0 sections 7 and 8 already disclaim warranty and limit liability more thoroughly than MIT does — naming the legal theories including negligence, enumerating damage types, and holding even where the contributor was advised of the possibility — but that language sits inside a 202-line file nobody evaluating the tool will open. The README note adds visibility, not legal force, and mirrors the license's own terms so it cannot be read as offering different ones. Generalize the "Relationship to Citadel" ADR. Radius Method no longer owns Citadel, so describing it as "Radius Method's" is now factually wrong, and the ADR additionally named a private repository along with its stack and deployment environment. The architectural point — that this tool governs what an agent can reach, while a gateway governs how it gets called — is generic to any LLM proxy, so nothing is lost by making it generic. There was never any code coupling: no import, no config, no runtime interaction, and the compiled binaries contain no such string. Note for the pre-publication decision: removing the reference from HEAD does not remove it from history. Commits d8d8f06 and 1f02bed still carry the original text, and `git log -S citadel --all` will surface it once the repository is public. Crow-Session: E3CF07BB-A433-4ABA-9A23-2A3FB3B61671 Co-Authored-By: Claude <noreply@anthropic.com>
|
Third commit — 1. Copyright entity was wrong
Verified the correction didn't damage the canonical text: Exactly one line differs, and it's the appendix placeholder that's supposed to be filled in. Still 202 lines, so GitHub license detection, A sweep confirmed this was the only legal-context occurrence in the repo — no NOTICE/DISCLAIMER/COPYING/AUTHORS file, no copyright notice outside 2. Warranty disclaimer surfaced in the READMEThe requirement is that the project carry no liability for what could go wrong from its usage. Apache-2.0 already satisfies this, and more thoroughly than MIT would:
MIT compresses this into one paragraph; Apache-2.0 names the legal theories, enumerates damage types, and adds §9. No license change was warranted — the gap was visibility, not coverage, since §7/§8 sit inside a 202-line file nobody opens. So the README's 3. "Relationship to Citadel" ADR generalized
Radius Method no longer owns Citadel, so the ownership claim is now factually wrong, independent of the disclosure question. The ADR's actual point is generic to any LLM proxy, so it's now stated generically:
The scope-boundary reasoning a contributor would want is preserved; nothing is named. There was never any code coupling — no import, no config, no runtime interaction, and
|
strings.Contains(s, "") is true for every s. Before the default host was removed, client.Host was never empty and both hook match sites were always meaningful. Removing the default made host == "" a reachable state, which turned `glk hook run` into a blanket deny: every WebFetch matched, and every command whose first word is curl or glab matched regardless of target. The block message rendered the empty host, reading "Use glk for access." The failure was sticky and undiagnosable. A user who runs `glk install-hooks` before `glk init` gets the hook written into ~/.claude/settings.json, and every WebFetch and curl in their session is rejected with exit 2 and a message naming no host. That is the same silent-misconfiguration mode this branch set out to remove, relocated from the API client into the hook — and a safety control that blocks everything is one users disable, which turns an availability problem into an exposure one. The hook now fails open with no configured host, matching the file's existing convention for unreadable and malformed stdin. Three tests cover it; verified the control still blocks the configured host (exit 2) and still allows unrelated traffic. resolveProject and resolveProjectArgs called DetectProject(client.Host) before any request, so an unconfigured auto-detect reported `no git remote matching host ""` rather than the actionable error. Auto-detection inside a checkout is the documented workflow, so that wrong diagnosis was the first one most users would hit. Both now return ErrNoHost. Audited the remaining non-request consumers of the host value rather than only the request builders. glkcli/job_trace.go collapses its cache path when the host is empty, but nothing can write to the collapsed path — the write happens after a fetch that now fails — so the read misses and falls through to the guarded request; verified end-to-end. greccli/locate.go degrades from exact lookup to fuzzy search, which is correct: grec is local-only and must not require a GitLab host. Broaden the ADR invariant accordingly. The rule that would have caught this is about every consumer of the host value, not only methods that build requests: builders fail closed and loudly, while matchers and detectors fail open or invert silently and need their own decision about which. Update the work-items search URL to the current canonical shape; docs.gitlab.com has dropped the /ee/ prefix and .html suffix. Crow-Session: E3CF07BB-A433-4ABA-9A23-2A3FB3B61671 Co-Authored-By: Claude <noreply@anthropic.com>
|
The Red is a genuine regression I introduced, and your root cause is exactly right. Fixed in 🔴 Hook blanket-deny — fixedReproduced before touching anything. Wrote the failing tests first: Same double-space artifact you saw. Fixed with the early return you suggested, matching the file's existing fail-open convention: // Fail open when no host is configured. There is no default host, so
// this is a reachable state — and both match paths below use
// strings.Contains, which returns true for an empty needle.
if host == "" {
return nil
}All 13 hook tests pass. Verified against the built binary that the control is intact — it still blocks the configured host and still allows unrelated traffic: Your framing of the severity is the part I'd underweighted: it fails closed on unrelated traffic so nothing leaks, but a control that blocks everything is one users disable — which is how an availability problem becomes an exposure one. 🟡 Auto-detect error — fixedBoth Agreed this was the more visible of the two, since running inside a checkout is the documented workflow. 🟡 Empty-host coverage — added, and I re-ran the audit properlyFive new tests: three hook, two More importantly, I redid the audit the way it should have been done the first time. Rather than grepping for request builders, I enumerated every consumer of
🟢 ADR invariant broadenedYou're right that my rule was scoped to the wrong thing. It now reads:
🟢 Docs URLConfirmed — 🟢 Dead
|
dhilgaertner
left a comment
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Third pass. Both previously-blocking findings are fixed, and I verified each against make build rather than taking the diff at its word. Nothing Red or Yellow remains.
Previous findings — both resolved
🟢 Client.GraphQL chokepoint (round 1) — fixed at internal/gitlab/client.go:325. The path that produced a 1.2 KB URL-encoded transport error now returns ErrNoHost:
$ GLK_CONFIG=/nonexistent/config.toml GITLAB_HOST= ./glk epic notes big-bang 123
no GitLab host configured — set gitlab.host in your config file (run `glk init`) or set GITLAB_HOST
exit=1
I re-audited exhaustiveness rather than assuming it: rg 'url\.URL\{|http\.NewRequest|\.Do\(' across internal/, glkcli/, greccli/ (non-test) returns request construction only at client.go:122 (Get), :198 (getStream), :247 (the already-validated redirect inside getStream), and :340 (GraphQL) — all guarded — plus doctor.go:760,795, which target github.com for the release check and are correctly unrelated to the GitLab host. internal/index derives hosts from local git remotes, not config, so the spider needs no guard.
🟢 glk hook run blanket-deny (round 2) — fixed at glkcli/hook.go:53. Fails open before either strings.Contains call, and confirmed both directions still behave:
# no host — unrelated traffic allowed
WebFetch https://example.com/docs → exit 0
curl https://example.com/api → exit 0
glab mr list → exit 0
# host configured — still blocks correctly, no regression
GITLAB_HOST=gitlab.example.com, WebFetch https://gitlab.example.com/x
→ "Use glk for gitlab.example.com access." exit 2
GITLAB_HOST=gitlab.example.com, WebFetch https://example.com/docs
→ exit 0
🟢 Auto-detect error (round 2) — fixed at glkcli/project_resolve.go:19,42. Both resolvers return ErrNoHost before reaching gitremote.DetectProject.
The ADR's closing paragraph was also broadened to cover every consumer of the host value, not just request builders, and it now articulates why the two failure shapes differ — request builders fail closed and loudly, matchers fail open or wrongly. That is the rule that would have caught the hook bug, and it is the right generalization to leave behind.
Security Review
Strengths:
- Removing the default host strengthens the pinned-host invariant rather than relaxing it — every request now goes to a host someone deliberately chose. Guards are at the request chokepoints, so
glk doctorcan still diagnose the condition it reports. grec checkoutrefuses before building a clone URL, mentioning--host(greccli/checkout.go:78), sohttps:///group/project.gitnever reachesgit.- The hook's fail-open is the correct call and is documented as a deliberate decision, not an oversight — a guardrail that blocks everything is one users uninstall. No security boundary is lost: with no configured host there is no host to protect.
artifactArchiveRedirectHosts[client.Host]with an empty host yields a nil map — fail-closed, and now documented as an allowlist rather than a default (glkcli/artifacts.go:25-31).- LICENSE is canonical Apache-2.0 (202 lines, appendix filled with
Copyright 2026 Radius Method, Inc.), declared in both README and the Homebrew formula.
Concerns: none. No credential handling, injection surface, or visibility check changed. Remaining repo1.dso.mil occurrences are test fixtures, recorded HTTP fixtures, and documented examples of a public instance — no disclosure regression.
Code Quality
🟢 Consider: glk doctor emits a confusing second problem when no host is set — glkcli/doctor.go:586
repoContextChecks calls detectProject(""), which cannot match any remote, so an unconfigured install reports two problems where there is one:
gitlab host error no GitLab host configured — hint: set gitlab.host ...
repo_context project warn no git remote matching host "" — provide a project argument
explicitly — hint: run inside a matching checkout or pass an
explicit project argument
The empty "" reads as a bug, and the hint is actively wrong — I ran this from inside a git checkout. Not blocking: the actionable root cause is on the line directly above, and doctor already exits non-zero from the host check alone, so nothing is missed. Short-circuiting repoContextChecks when state.cfg.GitLab.Host == "" (or emitting a skipped detail) would tighten it. Worth noting the new TestRunDoctor_MissingHostIsProblem stubs detectProject to succeed, so this line isn't covered either way.
Verification
Run locally on the merge head, all clean:
| Check | Result |
|---|---|
go vet ./... |
clean |
golangci-lint run ./... |
0 issues |
full suite (signed test path, -exec shell/go-test-codesign-exec.sh) |
all packages ok |
make check-skills |
clean — generated skill/ matches the template |
make build |
succeeds, both binaries signed |
New tests are well-targeted: ErrNoHost from all three chokepoints, message actionability (gitlab.host / glk init / GITLAB_HOST), grec checkout refusal asserting the --host mention, three hook fail-open cases, and both project_resolve entry points. Offline commands verified still working with no host: glk config show exits 0, glk init scaffolds.
Summary Table
| Color | Meaning | Verdict effect |
|---|---|---|
| Red | Must fix | Request changes |
| Yellow | Should fix | Request changes |
| Green | Consider | Approve allowed |
Recommendation: Approve — driven by [0 Red, 0 Yellow, 1 Green] findings.
The three remaining checkboxes in the PR body (visibility flip, go-licenses inventory, AGENTS.md/CLAUDE.md human pass) are correctly scoped out of this PR. Flipping visibility is irreversible in practice and is Danny's call, not this branch's.
2 participants
Closes #107
Open-source readiness work. This PR does not change repository visibility — that call is Danny's alone.
Decisions recorded
Three items in the ticket were flagged as decisions rather than defects. All three were confirmed before implementing:
repo1.dso.mildefault hostdocs-local/1. LICENSE — the hard blocker
Added Apache-2.0 (canonical text,
Copyright 2026 Radius Method, Inc.). Without this, publishing would not have made the repo open source: with no license, default copyright applies and nobody may use, modify, or redistribute it. README states the license; the Homebrew formula declareslicense "Apache-2.0".2. No default GitLab host
gitlab.hostwas defaulted torepo1.dso.mil. Two problems, one practical and one structural:glkquietly querying an instance they never chose. The failure surfaced as a confusing 404 against an unfamiliar host.The host is now required. Validation happens at the two request chokepoints (
Client.Get,Client.getStream), not at startup — so commands that need the network fail with an actionable error while offline commands keep working. That ordering matters:glk doctoris what you run to diagnose a missing host, so it must not itself require one.Verified end-to-end against the built binary:
Two bugs this surfaced
grec checkoutbuilt a hostless clone URL. With no host it producedhttps:///big-bang/bigbang.gitand handed it togit. It now refuses first, and mentions its--hostflag in the error.glk doctorreported an empty host as healthy —gitlab host ok (source: default). It now reportserrorand counts as a problem, sodoctorexits non-zero.repo1.dso.milremains a documented example and is unchanged in recorded HTTP fixtures.3–5. Disclosure and packaging
gitlab-kiosk-prd.md(internal product framing, 14dso.milreferences) anddocs-local/(managed-macOS execution-policy notes). Fixed the now-dangling PRD reference inARCHITECTURE.md.Makefiletap URL:ssh://git@github.com/...→ HTTPS, so an unauthenticated user canbrew install.GOPRIVATEinstructions (unnecessary for a public module) and a hardcoded personal path (/Users/daniel/radius/gitlab-kiosk→$(pwd)).work-itemssearch help pointed athttps://repo1.dso.mil/help/...; nowdocs.gitlab.com.skill/SKILL.mdheadline was "glk — GitLab CLI for repo1.dso.mil"; now host-neutral. Regenerated viamake gen-skills.6. Full-history secret scan — clean
Run over all 165 commits (this worktree has full history, not the shallow clone the audit used). Two independent scanners:
The single gitleaks hit is
generic-api-keyonglkcli/artifacts_test.go:203at commit2cb1291. The matched text is:The rule fired on the string literal
keycloak-0.txt(26 chars, entropy 3.52) following the wordkeycloak. It is a test asserting an extracted pod-log path — not a credential. TruffleHog, which verifies candidates against live providers, found nothing at all.No history rewrite is required.
Architecture
Per
CLAUDE.md, this touches more than 2–3 commands, so it needed architectural backing before implementation. The existing Pinned host invariant already supports it — the invariant is about pinning to the configured host, and removing the default strengthens rather than weakens it.ARCHITECTURE.mdgets:default: https://repo1.dso.mil)Verification
go vetclean ·golangci-lint0 issues · full suite passes via the repo's signed test path (got ./...) ·make check-skillsclean ·make buildsucceeds.New tests:
ErrNoHostfrom both chokepoints, error-message actionability,grec checkoutrefusal, anddoctorreporting a missing host as a problem.TestRunDoctor_Healthynow writes a real config file, since "healthy" now means a host is actually configured.TestLocate_DefaultHostInfersConfig→TestLocate_ConfiguredHostDisambiguates(the old name described a default that no longer exists).Remaining before going public
go-licensesinventory for the record. Dependencies are all permissive (MIT / Apache-2.0 / BSD-3), no copyleft.AGENTS.md/CLAUDE.mda read for internal workflow references — publishable in principle, but worth a human pass.🐦⬛ Generated with Claude Code, orchestrated by Crow