feat(gh): commit and tag through the API, with no credential on disk

[totollygeek]

First of two. This adds the capability; the follow-up moves @zuke/ai's lint fixer onto it and drops persist-credentials from the gate job.

The problem

CI that commits usually checks out with persist-credentials, commits, and pushes. That writes the token into .git/config, where it outlives the step that needed it: every later step can read it, and anything archiving the workspace carries it out.

Blocking a job's egress does not answer this, and I claimed otherwise in #312 before correcting myself. The token is a GitHub credential and GitHub is necessarily on any allowlist, so a block bounds where it could be sent, not what it could do.

Placement — corrected twice

I put this in @zuke/core first, on the reasoning that @zuke/ai has no HTTP and would need a new dependency to reach @zuke/gh. That premise was false: @zuke/ai has a src/hosts/ directory of GitHub, GitLab, Azure and Bitbucket API clients, because the reviewer posts comments to whichever host it runs on. With it gone the argument is one-sided — @zuke/gh is the GitHub package and already makes direct REST calls with a fetch seam.

The move also fixed a guideline the core version broke: a package exposes operations through a namespaced *Tasks object configured by a settings lambda, not loose functions taking an options bag.

Then the core floor check caught the second half of it. Reaching for a new option on core's HTTP helper needed a floor @zuke/gh does not declare, which would have needed a core release first. uploadSarif — the sibling operation in this same package — calls its own fetch seam directly and builds its own error. This now does the same, so @zuke/core is byte-identical to master and there is no sequencing constraint at all.

The API

Committing onto an existing branch names the branch, the message and the files. Adding a base branch creates the branch instead of moving it. Tagging names the tag and the commit, and asking to move repoints an existing one. Both take owner/repo and the token from the Actions environment when not set, as uploadSarif already does.

Two deliberate asymmetries, both asserted in tests:

• The branch update is unforced — a commit landing between reading the head and writing it is rejected rather than silently overwritten.
• A moved tag is forced, because pointing a major tag at a newer release is a non-fast-forward by definition. Moving one that does not exist yet creates it, since for a first release those are the same intent.

A real defect this found

Review finding 2y2yj4phucoli on #312 asked what happens if a branch name is not validated. Tracing it turned up worse than a malformed name: these go into request paths, and URL normalisation resolves dot segments before the request is sent. A branch name of ../../../user/repos turns the ref path into /repos/owner/name/user/repos — a different endpoint, with a write-scoped token attached.

Every entry point now validates by git's own ref rules and refuses before anything is sent, because a request that goes out and fails has already carried the token somewhere unintended. The test asserts zero calls were made, and asserts the normalisation itself so the reason survives the code.

What this does not claim

The token is still readable by code running in the step that uses it — that step is what uses it, and nothing short of not having a token avoids it. What is removed is the credential's persistence beyond its use. The module says so itself rather than leaving a reader to infer more.

8 tests, fetch injected, nothing touches the network. ./zuke ci green, 18/18.

🤖 Generated with Claude Code

[github-actions]

🤖 Zuke AI review

🔎 security review — review

Score: 0/10 · Severity: none · 0 finding(s)

Tokens: 15754 in · 51 out · 15805 total

The diff adds GitHub commit/tag API helpers with ref and repo slug validation, and I found no new exploitable security issues introduced by these changes.

[github-actions]

🤖 Zuke AI review

🔎 generic review — review

Score: 3/10 · Severity: low · 3 finding(s)

Tokens: 15914 in · 229 out · 19182 total

Severity	Finding	Location
low	Use of forbidden type assertions (as) in source code	packages/gh/src/commit.ts:492
low	Use of forbidden type assertions (as) in test file	packages/gh/tests/commit_test.ts:36
low	Public API exposes loose functions instead of namespaced tasks	packages/gh/mod.ts:21

Dismiss a false positive

Add a finding's ID to the suppress list to hide it next time:

• 9d0nf96jpjg9 — Use of forbidden type assertions (as) in source code
• 2qs2iar4nh1m7 — Use of forbidden type assertions (as) in test file
• 2dbg8h4gjshrg — Public API exposes loose functions instead of namespaced tasks

The changes violate strict project guidelines by using type assertions (as) and exposing loose functions instead of namespaced tasks in the public API.

[totollygeek]

Two adversarial reviews ran against this branch and #312. Everything they confirmed here is fixed in 84e2d2d; the serious findings landed on #312, which I have marked draft.

Fixed — the ref validator was bypassable

The finding I thought I had closed. The validator rejects a literal .., but % is legal in a git ref, so %2e%2e passed it — and the URL parser decodes that to a double-dot segment and resolves it. Reproduced end to end: a branch name of %2e%2e/%2e%2e/user/repos sent a token-bearing GET and PATCH to a path outside the repository the caller named.

Validation was the wrong instrument for this. Each slash-separated segment is now percent-encoded, so %2e%2e becomes an ordinary segment name. That also covers the repository slug, which was interpolated into the same path and never validated at all — the same primitive, one field over, which I had missed entirely.

Segment-wise rather than wholesale, because a slash inside a branch name is meaningful: chore/action-v1.0.3 has to stay three path segments. The validator stays too, since a name git itself would refuse deserves the clearer error. Three tests, including one asserting every request stays under the named repository and one asserting real branch names survive.

Fixed — a bare catch reported the wrong failure

Moving a tag recovered from a missing ref with a bare catch, which also swallowed 401 and 403. An expired token or a missing permission was retried as a create and surfaced as a ref conflict — the wrong error, hiding the one that mattered, in a path that runs unattended. Failures now carry their status and only a missing ref recovers. Tested with a 403, asserting it propagates and that no create is attempted.

Fixed — a stray import block inside the module's doc example

An editing mistake of mine had pasted an import block into the ```ts fence in gh.ts. Caught before `apiDocs` carried it into the README and `llms-full.txt`.

Moot — the core changes are gone

The reviews flagged HttpOptions.body without method throwing a raw TypeError, and the new core surface shipping without direct tests. Both applied to the earlier shape of this PR. @zuke/core is now byte-identical to master, so neither exists.

Noted, not fixed here

commitFiles with an empty file list is unspecified — GitHub either rejects it or creates an empty commit, and I could not determine which offline, so I have not asserted a behaviour I have not seen. And file paths in a commit body are unvalidated, unlike ref names; they go into a JSON body rather than a request path, so there is no traversal of the API itself. Both are worth settling before a caller depends on them, and neither has one yet.

./zuke ci green, 18/18. 12 tests.

[totollygeek]

Three findings this run. One was a real violation with a real consequence; two are refuted by this package's own established pattern.

9d0nf96jpjg9 — "forbidden type assertions in source code" (low) — correct, and understated. Fixed in eb7a7d3.

The caller was generic over a response type and ended in as T. The ban on that is not stylistic here: asserting the shape meant a field GitHub renamed, or omitted on some error path, would flow onward as undefined — into a request path, or into a tag ref — with nothing saying which call had returned something unexpected. A request to /git/commits/undefined is a confusing 404, not a diagnosis.

It returns unknown now, and each field is read through a checked accessor that names the response it came from. The last assertion — indexing after a typeof check — is a type guard, which is what "narrow rather than force" means and what makes the accessor safe rather than merely quiet.

2dbg8h4gjshrg — "public API exposes loose functions instead of namespaced tasks" (low) — refuted by this package's own pattern

packages/gh/mod.ts already exports mintAppToken and uploadSarifReport alongside GhTasks, for the same reason: the task object is the intended entry point, and the underlying function stays exported so it can be called and tested directly. commitFiles and tagCommit follow that exactly, and both are reachable as GhTasks.commit and GhTasks.tag.

This finding was right when it was raised against the earlier shape of this PR, where the operations lived in @zuke/core as loose functions with no task object at all. Moving them here is what fixed it.

2qs2iar4nh1m7 — "forbidden type assertions in test file" (low) — refuted by precedent, and I would rather not special-case it

The four occurrences are as typeof fetch on fake transports. packages/ai/tests/agent_fixer_test.ts does the identical thing three times for the identical reason: a hand-written stub cannot structurally satisfy fetch's full overloaded signature, and the alternative is a much larger fake that tests nothing extra.

If the guideline should cover tests too, that is a repository-wide change rather than one this PR should make unilaterally — and I would rather raise it than quietly diverge from the file next door.

All 10 checks pass; security review 0 findings. ./zuke ci green, 18/18, 12 tests.

[totollygeek]

Latest run: security review 1 finding, generic review skipped — gemini API error: HTTP 429, so the generic comment above is stale (it is still the body from 84e2d2d, whose three findings are answered in my previous comment).

tunixxmwrzt7 — "repository slug is not validated before being interpolated into API paths" (low) — confirmed in substance, fixed in a5ee086.

The cited line is the tree body rather than a request path, but the claim underneath it is right and I had missed it: the slug is percent-encoded and never validated, unlike the ref names immediately beside it.

Worth being precise about what the gap was, because it is not the one the finding's title suggests. Encoding already stops the slug climbing out of /repos/ — a dot segment survives as a literal name rather than resolving, which is exactly the fix from the previous round. What encoding does not constrain is the number of segments. A slug of a/b/c builds /repos/a/b/c/git/trees: still under /repos/, still not traversal, but an endpoint the caller never named, reached with a token attached.

So the guard is a shape check, not a traversal check: exactly two non-empty segments. It sits in the caller rather than in each settings class, because every request routes through the caller and a check in either settings class alone would have been one path short — commitFiles and tagCommit each build their own.

The existing encoding test used a four-segment slug to demonstrate escaping, which the guard now refuses, so it was narrowed to a two-segment slug that still carries %2e%2e — the encoding assertion is unchanged and now also asserts every request stayed under the encoded slug. One new test covers the refused shapes, with a transport that throws if any request is attempted, so it fails if the guard stops running rather than passing vacuously.

./zuke ci green, 18/18. 13 tests in packages/gh/tests/commit_test.ts.

[totollygeek]

3sxiscnkq8auw — "missing explicit handling for non-JSON error bodies in REST failures" (low) — half right, and the half that was right is fixed in 9e525a0.

The error path was already safe, and deliberately so: a failing response is never parsed. The body goes verbatim into GhApiError, because when GitHub refuses, its own message is the useful half and parsing it would risk losing it.

The success path is where the gap was. It parsed bare, so a 2xx whose body is not JSON — a proxy or gateway answering instead of GitHub, an interstitial from a corporate egress filter — surfaced as a SyntaxError naming no call at all. That is the same dead end readString was added to close one round ago, so leaving it would have been inconsistent within the same function.

One detail worth flagging, since it is the part that could have gone quietly wrong. The wrapper is intentionally not a GhApiError. That type means GitHub refused, and tagCommit reads its status to decide whether a missing ref should be created instead. Reusing it for a parse failure would have fed an unrelated error into that branch and retried it as a tag create — reintroducing, by a different route, exactly the bug the typed error was added to fix last round. There is a test asserting no create is attempted.

Two tests: one asserting the message names the call, says the body was not JSON, and carries a prefix of what actually came back; one asserting a non-JSON body never enters the create-the-missing-ref path.

Also worth noting for anyone reading the review comments above: the generic review has skipped its last three runs with gemini API error: HTTP 429, so that comment is still the body from 84e2d2d. Its three findings were answered earlier — one fixed, two refuted on this package's own precedent — and nothing in it reflects the last three commits.

./zuke ci green, 18/18. 15 tests in packages/gh/tests/commit_test.ts.