feat(#2735): replace GitHub Contents API with git sparse checkout

[ggallen]

Summary

• Introduces internal/gitfetch package that fetches directory trees via git sparse checkout (--filter=blob:none --depth 1), replacing GitHub Contents API calls (ListDirectoryContents + GetFileContentAtRef)
• Adds CloneURL() method to ForgeURLInfo for forge-agnostic clone URL construction (maps forge names → hostnames)
• Migrates all skill-directory fetch call sites (compose.go, resolve.go, fetchsvc/service.go, run.go, lock.go) from ForgeClient to TreeFetcher gitfetch.TreeFetchFunc + GitToken string
• Removes GH_TOKEN injection from action.yml "Run fullsend" step — the ambient GITHUB_TOKEN is resolved by resolveToken() at runtime

Motivation

This eliminates the chicken-and-egg problem described in #2722: LoadWithBase() needs a token to fetch skill directories via the GitHub Contents API, but the mint role needed to obtain that token is only known after loading the base. PR #2720 worked around this by injecting GH_TOKEN, but that's over-privileged.

With git sparse checkout:

• Public repos need no token at all
• Private repos use the ambient GITHUB_TOKEN (always available in Actions)
• The approach is forge-agnostic, preparing the codebase for GitLab support

What stays unchanged

• forge.Client interface — ListDirectoryContents / GetFileContentAtRef remain (used by DiscoverRemoteAgents)
• FakeClient — stays for discover_remote_test.go

Test plan

• internal/gitfetch package: 86.1% coverage (>85% threshold)
• All existing tests updated to use TreeFetchFunc instead of FakeClient
• go test ./internal/gitfetch/ ./internal/harness/ ./internal/resolve/ ./internal/fetchsvc/ ./internal/forge/ ./internal/cli/ — all pass
• go vet — clean on all changed packages
• Pre-commit hooks pass (gofmt, pinact, etc.)

Closes #2735
Refs #2722, #2720

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Replace GitHub Contents API directory fetch with git sparse checkout
✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Description

• Introduce forge-agnostic directory fetching via git sparse checkout (no Contents API).
• Thread TreeFetcher + optional GitToken through compose/resolve/runtime fetch paths.
• Remove GH_TOKEN workflow injection; rely on runtime token resolution when needed.

Diagram

graph TD
  CLI(["CLI (run/lock)"]) --> Harness["harness.Compose / LoadWithBase"] --> ForgeURL["forge.ForgeURLInfo.CloneURL"] --> GitFetch["gitfetch.FetchTree"] --> GitExec{{"git (sparse checkout)"}} --> Repo[("Remote repo")]
  CLI --> Resolve["resolve.ResolveHarness"] --> ForgeURL
  CLI --> FetchSvc["fetchsvc.Service"] --> ForgeURL

  subgraph Legend
    direction LR
    _cli(["Caller"]) ~~~ _mod["Module"] ~~~ _ext{{"External tool"}} ~~~ _repo[("Repo")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use forge "tree" APIs (GitHub Git Trees / GitLab repository tree)

• ➕ Avoids requiring local git binary execution
• ➕ Potentially faster for small trees; fewer subprocesses/temp dirs
• ➕ No need to embed tokens into clone URLs
• ➖ Reintroduces forge-specific API surface and auth bootstrapping complexity
• ➖ May still require elevated tokens for private repos
• ➖ More divergent implementations across forges

2. Download repo tarball/zip for a ref and extract subdir

• ➕ Simple HTTP fetch; no git commands needed
• ➕ Often supported across forges
• ➖ Downloads more data than needed (worse for large repos)
• ➖ Subdir extraction adds complexity and potential path traversal concerns
• ➖ May have weaker caching granularity than per-skill directory trees

3. Use git worktree + persistent local mirror cache

• ➕ Avoids repeated network fetches across multiple skill resolutions
• ➕ Could improve performance for repeated runs in CI runners with cache
• ➖ More complex lifecycle/locking and cache invalidation
• ➖ Higher risk of cross-run contamination if not carefully isolated
• ➖ Bigger implementation surface than this PR

Recommendation: The sparse-checkout TreeFetcher approach is a good strategic fit: it removes the Contents API token bootstrapping problem, is naturally forge-agnostic, and keeps the fetch semantics consistent across compose/resolve/runtime fetch. If performance becomes an issue, consider adding an optional persistent mirror cache, but the current design is the right complexity/performance tradeoff for correctness and portability.

Files changed (15) +861 / -382

Enhancement (3) +194 / -21

service.goRuntime fetch service now fetches uncached skills via TreeFetcher +13/-21

Runtime fetch service now fetches uncached skills via TreeFetcher

• Replaces ListDirectoryContents/GetFileContentAtRef logic with a single TreeFetcher call that returns all files in a directory tree. Adds GitToken storage and defaults TreeFetcher to gitfetch.FetchTree when unset.

internal/fetchsvc/service.go

url.goAdd forge-agnostic CloneURL() for repository cloning +16/-0

Add forge-agnostic CloneURL() for repository cloning

• Introduces ForgeURLInfo.CloneURL() and a small forge→host mapping helper (github/gitlab, default passthrough). Enables callers to build HTTPS clone URLs consistently from parsed forge info.

internal/forge/url.go

gitfetch.goNew gitfetch package: sparse-checkout directory tree fetching +165/-0

New gitfetch package: sparse-checkout directory tree fetching

• Implements FetchTree using a temp shallow clone with blob filtering and sparse checkout, then walks the checked-out directory to return file contents. Includes safety limits (MaxFiles, max total bytes) plus token embedding and redaction helpers.

internal/gitfetch/gitfetch.go

Refactor (4) +64 / -114

lock.goSwitch lock command to TreeFetcher + GitToken plumbing +10/-21

Switch lock command to TreeFetcher + GitToken plumbing

• Replaces ForgeClient construction/usage with an optional git token resolved via resolveToken(). Passes TreeFetcher and GitToken into harness composition and dependency resolution paths.

internal/cli/lock.go

run.goMigrate run command and fetch-service setup to git-based fetching +21/-27

Migrate run command and fetch-service setup to git-based fetching

• Replaces ForgeClient-based token/client resolution with TreeFetcher + GitToken handling. Updates setupFetchService to configure runtime fetching with a git token (resolved lazily when needed).

internal/cli/run.go

compose.goUse TreeFetcher (git sparse checkout) for URL-base skill composition +19/-39

Use TreeFetcher (git sparse checkout) for URL-base skill composition

• Replaces ForgeClient-based full-directory fetch with TreeFetcher + optional GitToken, defaulting to gitfetch.FetchTree. Adjusts cache staleness behavior to refetch when online regardless of a forge client.

internal/harness/compose.go

resolve.goResolve URL skill directories via TreeFetcher instead of forge APIs +14/-27

Resolve URL skill directories via TreeFetcher instead of forge APIs

• Replaces directory listing + per-file fetch with a single TreeFetcher call (default gitfetch.FetchTree) and optional GitToken. Preserves existing hashing/integrity and cache behavior while removing the ForgeClient requirement for uncached fetches.

internal/resolve/resolve.go

Tests (7) +603 / -246

lock_test.goUpdate lock tests to stub TreeFetchFunc instead of FakeClient +7/-15

Update lock tests to stub TreeFetchFunc instead of FakeClient

• Reworks tests to provide a fake TreeFetcher returning skill files, removing reliance on forge.FakeClient directory/file API maps. Ensures lock flow continues to work with the new fetch abstraction.

internal/cli/lock_test.go

run_test.goAdapt run tests to TreeFetcher/GitToken fetch-service configuration +12/-10

Adapt run tests to TreeFetcher/GitToken fetch-service configuration

• Renames and rewrites fetch-service setup tests to validate TreeFetcher injection and token resolution behavior. Removes the now-unused mockForgeClient type.

internal/cli/run_test.go

service_test.goRewrite fetch service tests around TreeFetcher semantics +20/-27

Rewrite fetch service tests around TreeFetcher semantics

• Removes FakeClient fixtures and introduces TreeFetcher stubs for directory returns and failure injection. Updates error expectations to reference directory fetch failures and preserves rate-limit rollback assertions.

internal/fetchsvc/service_test.go

url_test.goAdd unit tests for CloneURL() host mapping behavior +29/-0

Add unit tests for CloneURL() host mapping behavior

• Adds coverage for GitHub, GitLab, and unknown-forge host passthrough cases to validate clone URL construction.

internal/forge/url_test.go

gitfetch_test.goComprehensive tests for gitfetch FetchTree and helpers +360/-0

Comprehensive tests for gitfetch FetchTree and helpers

• Adds an on-disk git-repo test harness and validates FetchTree for branches/tags, root/subdir paths, error cases, size/file limits, context cancellation/timeouts, stderr capture, and token redaction/URL embedding.

internal/gitfetch/gitfetch_test.go

compose_test.goUpdate compose tests to use TreeFetcher and new error semantics +63/-80

Update compose tests to use TreeFetcher and new error semantics

• Introduces helper TreeFetcher fakes and rewrites tests that previously depended on forge.FakeClient. Updates expectations around default fetcher usage, parse errors, stale cache refetching, and full-directory skill fetch behavior.

internal/harness/compose_test.go

resolve_test.goRefactor resolver tests to a path-routing TreeFetcher registry +112/-114

Refactor resolver tests to a path-routing TreeFetcher registry

• Replaces forge.FakeClient directory/file maps with a skillRegistry that returns a TreeFetchFunc serving pre-registered paths. Updates all resolver tests to pass TreeFetcher and adds explicit TreeFetcher error propagation coverage.

internal/resolve/resolve_test.go

Other (1) +0 / -1

action.ymlStop injecting GH_TOKEN into the action runtime env +0/-1

Stop injecting GH_TOKEN into the action runtime env

• Removes GH_TOKEN from the environment for the "Run fullsend" step. This aligns the workflow with runtime token resolution (ambient GITHUB_TOKEN when available) rather than passing an explicit input token.

action.yml

[github-actions]

Site preview

Preview: https://3bec34ed-site.fullsend-ai.workers.dev

Commit: fc0d4791c33419a7bd358f29dc2bc91a8d2bec4d

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:53 PM UTC · Completed 8:09 PM UTC
Commit: c093313 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 mint: add loader role so the binary self-mints tokens for URL-based harness loading 🎫 refactor(forge): replace GitHub Contents API with git-based directory fetching

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Tempdir escape via path 🐞 Bug ⛨ Security

Description

internal/gitfetch.FetchTree builds walkRoot with filepath.Join(tmpDir, path) and then walks/reads
from it without rejecting '..' or otherwise ensuring the resolved path stays under tmpDir. Because
the path comes from parsed URLs (ForgeURLInfo.Path), a crafted URL can escape the temp worktree and
read arbitrary host directories/files, which are then returned/cached as fetched content.

Code

internal/gitfetch/gitfetch.go[R73-81]

+	walkRoot := tmpDir
+	if path != "" {
+		walkRoot = filepath.Join(tmpDir, filepath.FromSlash(path))
+	}
+
+	info, err := os.Stat(walkRoot)
+	if err != nil || !info.IsDir() {
+		return nil, fmt.Errorf("gitfetch: path %q not found in repository at ref %s", path, ref)
+	}

Relevance

⭐⭐⭐ High

Repo has accepted path-containment hardening patterns (e.g., escape/traversal/symlink containment
fixes in PR #1177).

PR-#1177

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

FetchTree constructs a local filesystem root from the untrusted path and then stats/walks it;
upstream URL parsers do not reject traversal segments, and multiple call sites pass the parsed path
directly into the TreeFetcher.

internal/gitfetch/gitfetch.go[73-81]
internal/gitfetch/gitfetch.go[85-118]
internal/forge/url.go[48-92]
internal/forge/url.go[120-158]
internal/resolve/resolve.go[313-339]
internal/fetchsvc/service.go[129-166]
internal/harness/compose.go[829-842]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`gitfetch.FetchTree` trusts the `path` argument when building `walkRoot` via `filepath.Join(tmpDir, filepath.FromSlash(path))`. If `path` contains traversal segments (e.g. `../..`) or is absolute, the resulting `walkRoot` can point outside `tmpDir`, and the subsequent `os.Stat`/`filepath.Walk` will read from the host filesystem.

## Issue Context
The `path` value is derived from URL parsing (`forge.ParseForgeURL` / `forge.ParseRawContentURL`) and is passed through from multiple call sites (resolve, compose, fetchsvc). Those parsers currently join URL segments without rejecting `.`/`..`.

## Fix Focus Areas
- internal/gitfetch/gitfetch.go[57-81]
- internal/forge/url.go[48-92]
- internal/forge/url.go[120-158]

## Suggested fix
1. Normalize and validate `path` before using it:
  - Reject absolute paths.
  - Clean with `path.Clean` (POSIX) or equivalent and reject any result containing `..` path elements or that becomes `.`.
2. After computing `walkRoot`, enforce confinement:
  - Compute `absWalkRoot := filepath.Clean(walkRoot)` and `absTmp := filepath.Clean(tmpDir)` and ensure `absWalkRoot` has prefix `absTmp + string(os.PathSeparator)` (or equals `absTmp` only when `path == ""`).
3. Use the sanitized path both for `git sparse-checkout set` and for `walkRoot`.
4. Optionally add similar validation in `forge.ParseForgeURL` / `ParseRawContentURL` so invalid/traversal paths are rejected closer to the trust boundary.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Symlink allows host read 🐞 Bug ⛨ Security

Description

internal/gitfetch.FetchTree reads each visited file with os.ReadFile without rejecting symlinks, so
a repository can include a symlink whose target is outside the checkout directory and cause
arbitrary host file reads. The bytes are then returned as fetched tree content (and callers
cache/upload them), turning this into a serious local file disclosure vector.

Code

internal/gitfetch/gitfetch.go[R85-110]

+	walkErr := filepath.Walk(walkRoot, func(p string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if info.IsDir() {
+			if info.Name() == ".git" {
+				return filepath.SkipDir
+			}
+			return nil
+		}
+
+		rel, err := filepath.Rel(walkRoot, p)
+		if err != nil {
+			return err
+		}
+		rel = filepath.ToSlash(rel)
+
+		if len(files) >= MaxFiles {
+			return fmt.Errorf("directory listing exceeded maximum of %d files", MaxFiles)
+		}
+
+		data, err := os.ReadFile(p)
+		if err != nil {
+			return fmt.Errorf("reading %s: %w", rel, err)
+		}
+

Relevance

⭐⭐⭐ High

Team previously required rejecting symlinks/non-regular files before ReadFile for security (accepted
in PR #1177, #215).

PR-#1177
PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation walks the worktree and calls os.ReadFile on every non-directory entry, with no
check for ModeSymlink or other special-file types.

internal/gitfetch/gitfetch.go[85-117]
internal/gitfetch/gitfetch.go[106-116]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`gitfetch.FetchTree` uses `filepath.Walk` and then `os.ReadFile(p)` for all non-directory entries. If a checked-out entry is a symlink, `os.ReadFile` follows it and reads the target, which can be outside the checkout.

## Issue Context
Fetched repositories are not necessarily trusted; even with allowlisting, a compromised repo can introduce symlinks to sensitive host paths.

## Fix Focus Areas
- internal/gitfetch/gitfetch.go[85-117]

## Suggested fix
- In the walk callback, detect symlinks via `info.Mode()&os.ModeSymlink != 0` and fail the fetch (or skip, but failing is safer) with a clear error like "symlinks are not supported".
- Consider also rejecting other special file types (devices, named pipes) by checking `info.Mode().Type()`.
- Add/adjust tests to include a repo containing a symlink and assert FetchTree errors.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Token leaked in git URL 🐞 Bug ⛨ Security

Description

internal/gitfetch.authenticatedURL embeds the token into the clone URL and passes it as an argument
to git remote add, which can leak credentials via process listings and stores the secret in
.git/config for the duration of the fetch. Only git fetch errors are redacted; failures earlier
in the sequence may still surface the token-containing URL.

Code

internal/gitfetch/gitfetch.go[R40-56]

+	authURL, err := authenticatedURL(cloneURL, token)
+	if err != nil {
+		return nil, fmt.Errorf("gitfetch: %w", err)
+	}
+
+	tmpDir, err := os.MkdirTemp("", "fullsend-gitfetch-*")
+	if err != nil {
+		return nil, fmt.Errorf("gitfetch: creating temp dir: %w", err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	if err := runGit(ctx, tmpDir, "init"); err != nil {
+		return nil, fmt.Errorf("gitfetch: git init: %w", err)
+	}
+	if err := runGit(ctx, tmpDir, "remote", "add", "origin", authURL); err != nil {
+		return nil, fmt.Errorf("gitfetch: git remote add: %w", err)
+	}

Relevance

⭐⭐⭐ High

Team mitigates token leakage in args/logs (process-listing concern accepted in PR #337; token
redaction work in #736).

PR-#337
PR-#736

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly injects the token into the URL and uses that URL as a git command argument;
redaction is only applied to the fetch error path.

internal/gitfetch/gitfetch.go[40-56]
internal/gitfetch/gitfetch.go[66-68]
internal/gitfetch/gitfetch.go[148-157]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The git token is embedded into the remote URL (`https://x-access-token:*******@host/owner/repo.git`) and then passed to git as a CLI argument and written into the repo config, increasing the chance of accidental disclosure.

## Issue Context
Even though the workdir is temporary, credential-in-URL can leak through OS process inspection tools, verbose git output, or error strings from steps other than `git fetch`.

## Fix Focus Areas
- internal/gitfetch/gitfetch.go[40-68]
- internal/gitfetch/gitfetch.go[148-165]

## Suggested fix
- Do not put the token in the URL.
- Prefer one of:
 - `git -c http.extraHeader=AUTHORIZATION: bearer <token> ...` (or Basic with base64 as appropriate), or
 - `GIT_ASKPASS`/`SSH_ASKPASS` style helper that supplies the token via env/stdin.
- If keeping the URL approach temporarily, apply `redactToken(...)` to *all* git-step errors that could include the remote URL (remote add, sparse-checkout set, checkout), not only `git fetch`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. ghp_* token literals in tests 📘 Rule violation ⛨ Security

Description

The new unit tests include hardcoded token-like strings (ghp_test123, ghp_secret123), which can
be mistaken for real credentials and may trigger secret-scanning or policy violations. Sensitive
identifiers in source (including tests) should use clearly non-secret placeholder formats that do
not match real token prefixes.

Code

internal/gitfetch/gitfetch_test.go[R231-255]

+	}
+	want := "https://x-access-token:ghp_*****23@github.com/owner/repo.git"
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}
+
+func TestFetchTree_TokenEmbeddingEmpty(t *testing.T) {
+	got, err := authenticatedURL("https://github.com/owner/repo.git", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got != "https://github.com/owner/repo.git" {
+		t.Errorf("expected unchanged URL, got %q", got)
+	}
+}
+
+func TestRedactToken(t *testing.T) {
+	err := redactToken(fmt.Errorf("failed with token ghp_secret123 in output"), "ghp_secret123")
+	if strings.Contains(err.Error(), "ghp_secret123") {
+		t.Error("token not redacted")
+	}
+	if !strings.Contains(err.Error(), "***") {
+		t.Error("expected *** in redacted error")
+	}

Relevance

⭐ Low

Repo intentionally uses token-like prefixes in tests to validate redaction/normalization (accepted
PRs #736, #1178).

PR-#736
PR-#1178

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062040 flags token-like strings (including those starting with ghp_) as red
flags unless they are clearly fake placeholders; the added tests embed ghp_* literals directly in
expected URLs and error strings.

Rule 1062040: Disallow hardcoded secrets and sensitive environment-specific identifiers in source code
internal/gitfetch/gitfetch_test.go[231-235]
internal/gitfetch/gitfetch_test.go[248-255]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`internal/gitfetch/gitfetch_test.go` hardcodes token-like strings beginning with `ghp_` (e.g., `ghp_test123`, `ghp_secret123`). These resemble real GitHub PAT prefixes and can be misinterpreted as secrets or trigger automated scanners.

## Issue Context
The compliance rule prohibits hardcoded secrets and sensitive identifiers, including token-like strings; tests should use clearly fake placeholders that do not match real-world secret prefixes.

## Fix Focus Areas
- internal/gitfetch/gitfetch_test.go[231-255]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[codecov]

Codecov Report

❌ Patch coverage is 81.61435% with 41 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/gitfetch/gitfetch.go	77.04%	15 Missing and 13 partials ⚠️
internal/cli/run.go	60.86%	9 Missing ⚠️
internal/harness/compose.go	94.59%	1 Missing and 1 partial ⚠️
internal/resolve/resolve.go	75.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Looks good to me

---

Labels: PR already has appropriate component labels (component/harness, component/skills). The requires-manual-review label is present.

Previous run

Review

Findings

High

• [stale-test] internal/resolve/resolve_test.go:1182 — TestResolveHarness_NilForgeClientWithSkillURL is not updated in this PR. It asserts that the error message contains "ForgeClient is required" (line 1193), but the PR removes that error path from resolve.go — a nil TreeFetcher is now silently defaulted to gitfetch.FetchTree instead of returning an error. The test will still compile (it never references the ForgeClient field directly) but will fail at runtime because the expected error message no longer exists. The code now defaults to gitfetch.FetchTree which will attempt a real git fetch and fail with a different error (e.g., "fetching directory").
  Remediation: Either delete this test (the nil-fetcher-is-ok behavior is intentional and tested by TestFetchBaseSkill_DefaultTreeFetcherUsed) or update it to reflect the new behavior — assert on the new error message from the default gitfetch.FetchTree path.

---

Labels: PR already has appropriate component labels. The type/feature label matches the feat() prefix in the PR title.

Previous run (2)

Review

Findings

Medium

• [fail-open auth gate] internal/cli/run.go:311 — Token resolution failure downgraded from hard error to warning. Previously, when a harness contained URL-referenced skills and no GitHub token was available, the code returned a hard error (Skill URLs require a GitHub token). The new code emits printer.StepWarn("Git token not available; private repo skill fetches may fail") and continues with an empty token. The same pattern applies in lock.go (compose and resolve paths). For private repos this will fail at the git layer (network error), but for public repos content is fetched without authentication, bypassing any token-scoped access controls or audit trail tied to the token identity. This is an intentional design choice to support tokenless public-repo skill fetching, but the relaxation of the auth gate is worth noting.
  Remediation: Consider preserving the hard-error behavior when the harness explicitly references skills that require authentication, or log the unauthenticated fetch prominently in the audit log.

• [adr-consistency] docs/ADRs/0038-universal-harness-access.md — ADR-0038 is being updated to change references from "forge APIs" to "git sparse checkout" in the Skill directory model description, the Changes required section, and the Consequences section. Per AGENTS.md, accepted ADRs are point-in-time records — substantial rewrites to Context, Decision, or Consequences require a new superseding ADR. These changes modify the core technical mechanism described in the Decision section (forge APIs → git sparse checkout) and add a new paragraph about stale cache fallback, which goes beyond minor annotations.
  Remediation: Add an amendment section with date and reference to issue refactor(forge): replace GitHub Contents API with git-based directory fetching #2735, or create a superseding ADR that formally records the shift from forge APIs to git sparse checkout for skill directory fetching.

Previous run (3)

Review

Findings

Medium

• [error-handling-gap] internal/cli/run.go — In the resolve path of runAgent (the !usedLock branch), when rFlags.gitToken is empty, the token resolution error is silently discarded with resolveGitToken, _ = resolveToken(). The three other token resolution sites in this PR (both compose paths in run.go and lock.go, and the lock.go resolve path) all capture the error and warn via printer.StepWarn("Git token not available; private repo skill fetches may fail"). This inconsistency means users running fullsend run with URL skills targeting private repos get no warning before the fetch fails with a confusing git error in this specific code path.
  Remediation: Match the pattern used in the other three sites — capture the error and call printer.StepWarn when token resolution fails.

Previous run (4)

Review

Findings

Medium

• [behavior-change] internal/harness/compose.go — Skill directory fetching now falls back to stale cached content when re-fetch fails with a transient error (network timeout, DNS failure, connection refused, context deadline exceeded). Previously, a failed fetch was always a hard error. This is an intentional behavior change — agents could silently run with outdated skill definitions when the network is flaky. The fallback is well-tested (TestFetchBaseSkill_StaleCacheTransientFallback, TestFetchBaseSkill_StaleCacheContextDeadlineFallback, TestFetchBaseSkill_StaleCacheNonTransientError) and non-transient errors (auth failures) still propagate. The warning is attached to dep.Warning and surfaced via printResolvedDeps.

Previous run (5)

Review

Findings

Medium

• [behavior-change] internal/harness/compose.go — Skill directory fetching now falls back to stale cached content when re-fetch fails with a transient error (network timeout, DNS failure, connection refused, context deadline exceeded). Previously, a failed fetch was always a hard error. This is an intentional behavior change — agents could silently run with outdated skill definitions when the network is flaky. The fallback is well-tested (TestFetchBaseSkill_StaleCacheTransientFallback, TestFetchBaseSkill_StaleCacheContextDeadlineFallback, TestFetchBaseSkill_StaleCacheNonTransientError) and non-transient errors (auth failures) still propagate. The warning is attached to dep.Warning and surfaced via printResolvedDeps.

Previous run (6)

Review

Findings

Medium

• [logic-error] internal/gitfetch/gitfetch.go — The symlink check info.Mode()&os.ModeSymlink != 0 inside the filepath.Walk callback is dead code. filepath.Walk follows symlinks and reports the followed target's FileInfo, never setting ModeSymlink. This means symlinks in a fetched repository pointing outside the temp directory would be silently followed and their content included in the result map. While the risk is mitigated by the temp-dir context (the walked directory is a freshly cloned git repo), a malicious skill repository could include symlinks targeting sensitive host files.
  Remediation: Replace filepath.Walk with filepath.WalkDir and check d.Type()&os.ModeSymlink != 0 on the fs.DirEntry, or use os.Lstat before reading each file.

• [fail-open-auth] internal/cli/lock.go:306 — When resolving skill URLs for the lock command, token resolution failure is silently discarded: resolveGitToken, _ = resolveToken(). Previously the code returned a hard error (Skill URLs require a GitHub token) when HasURLSkills() was true and no token was available. For private repos, the git fetch will now fail at the network level with a confusing authentication error rather than the prior clear, actionable message. The compose path emits a StepWarn but the resolve/lock path does not.
  Remediation: Add a printer.StepWarn when resolveToken() fails and h.HasURLSkills() is true in the lock path, matching the compose path's behavior.

• [behavior-change] internal/harness/compose.go — Skill directory fetching now falls back to stale cached content when re-fetch fails with a transient error (network timeout, DNS failure, connection refused). This changes behavior from hard failure to graceful degradation with a warning. The fallback is well-tested (TestFetchBaseSkill_StaleCacheTransientFallback, TestFetchBaseSkill_StaleCacheContextDeadlineFallback, TestFetchBaseSkill_StaleCacheNonTransientError) and the Warning field propagates to callers via printResolvedDeps. This is a deliberate behavior change that should be documented in the ADR or release notes so operators understand the new failure mode.

Previous run (7)

Review

Findings

Medium

• [error-handling] internal/gitfetch/gitfetch.go — redactToken uses fmt.Errorf("%s", msg) which discards error wrapping (type information and errors.Is/errors.As chains are lost). Additionally, wrapTransient is not applied on all error return paths in FetchTree — git-checkout failures bypass it, so some transient errors may not trigger the stale-cache fallback in compose.go.
  Remediation: Preserve error chains by wrapping with %w after redaction (e.g., wrap the original error with a redacted message). Apply wrapTransient consistently to all git subprocess error paths in FetchTree.

• [logic-error] internal/scaffold/fullsend-repo/scripts/post-code.sh:456 — Removing the --label ready-for-review application changes the bot-to-human handoff signal for scaffolded repos. Downstream workflows or human reviewers relying on this label to detect "code review ready" will no longer be triggered.
  Remediation: Confirm no downstream automation or documented process depends on the ready-for-review label before removing it.

• [scope-creep] .github/workflows/reusable-dispatch.yml — The removal of ready-for-triage label routing, the triage-agent allowlist entry in config.go, and the --label ready-for-triage in post-retro.sh constitute a triage-agent deprecation that is orthogonal to the primary goal of replacing the forge API with git sparse checkout (issue refactor(forge): replace GitHub Contents API with git-based directory fetching #2735). This is coordinated and intentional but could be split into a separate PR for clearer review scope.
  See also: [scaffold-contract] findings for template-level impact.

• [scaffold-contract] internal/scaffold/fullsend-repo/scripts/post-code.sh — Removing --label ready-for-review from the scaffold template changes the contract for all newly scaffolded repositories. Existing repos scaffolded with the prior template retain the label logic, creating divergence between old and new scaffolded repos.

• [scaffold-contract] internal/scaffold/fullsend-repo/scripts/post-retro.sh — Removing --label ready-for-triage from the scaffold template similarly changes the scaffolded-repo contract for triage labeling.

• [protected-path] .github/workflows/reusable-dispatch.yml — This file is under .github/, a protected path. The change removes the ready-for-triage label routing block. Human approval is required regardless of automated review outcome.

Previous run (8)

Review

Findings

Medium

• [command-injection] internal/gitfetch/gitfetch.go:83 — The ref parameter is passed directly to git fetch --depth 1 --filter=blob:none origin <ref> without a -- separator. While exec.Command prevents shell injection, git itself interprets arguments starting with -- as options rather than refspecs. The FetchTree function is a public API (TreeFetchFunc type) that accepts arbitrary ref strings. Mitigating factors reduce practical risk: (1) URL scheme is restricted to https/file, (2) for HTTPS, --upload-pack has no effect (smart HTTP protocol ignores it), (3) file:// is only used in tests with hardcoded data, (4) production callers pass refs extracted from ParseForgeURL which produces structured commit SHAs and branch names. This is a defense-in-depth gap on the public API surface, not an exploitable vulnerability in current usage.
  Remediation: Add -- separator before the ref argument: runGitWithEnv(ctx, tmpDir, authEnv, "fetch", "--depth", "1", "--filter=blob:none", "origin", "--", ref). This is a trivial, zero-risk defensive fix.

Previous run (9)

Review

Findings

Medium

• [fail-open] internal/cli/lock.go:267, internal/cli/run.go — Token resolution errors are silently discarded with composeGitToken, _ = resolveToken() (and similarly resolveGitToken, _ = resolveToken() in three other locations). Previously, when URL skills were present and no token could be resolved, the code returned a hard error directing users to set GH_TOKEN. Now the code proceeds with an empty token, deferring the failure to the git fetch. This is not a security fail-open (private repos still fail with an auth error, not silent access), but the user experience is degraded for private-repo users: they see a git auth error instead of a clear pre-flight check. The new hint messages in compose.go and resolve.go ("hint: set GH_TOKEN or GITHUB_TOKEN for private repos") partially mitigate this, providing actionable guidance at the point of failure.
  Remediation: Consider logging a warning when resolveToken() fails and URL skills are present, so users get early signal that private-repo fetches may fail.

• [stale-reference] internal/resolve/resolve.go:75 — The ResolveHarness function's godoc comment still references "forge API (via ForgeClient)" for skill directory fetching. The PR updates the ResolveOpts struct fields (replacing ForgeClient with TreeFetcher/GitToken and their comments), and updates the implementation to use gitfetch.FetchTree, but does not update the function-level godoc block at lines 74-77 which still describes the old forge API approach.
  Remediation: Update the ResolveHarness godoc to reference gitfetch.FetchTree / git sparse checkout instead of "forge API (via ForgeClient)."

Previous run (10)

Review

Findings

Medium

• [command-injection] internal/gitfetch/gitfetch.go:46 — The cloneURL parameter is passed directly to git remote add origin <cloneURL> without validation that it uses a safe URL scheme. While upstream callers currently construct cloneURL via forgeInfo.CloneURL() (which produces https:// URLs), the FetchTree public API accepts an arbitrary string. A caller passing a clone URL using git's ext:: transport protocol could execute arbitrary commands. The ext:: transport is specifically designed to run external commands, making this a defense-in-depth gap on the public API surface.
  Remediation: Add URL scheme validation at the top of FetchTree to reject any cloneURL that is not https:// or file:// (for tests). For example: u, err := url.Parse(cloneURL); if err != nil || (u.Scheme != "https" && u.Scheme != "file") { return nil, fmt.Errorf("gitfetch: unsupported URL scheme") }

Previous run (11)

Review

Findings

Medium

• [error-handling] internal/harness/compose.go — isTransientFetchError checks errors.Is(err, context.DeadlineExceeded) and errors.Is(err, context.Canceled), but these sentinel checks will never match for errors produced by gitfetch.FetchTree in production. redactToken uses fmt.Errorf("%s", msg) which breaks the error chain, destroying the context sentinels. wrapTransient's string patterns also do not include deadline or canceled, so context errors will not produce a TransientError either. The result: context deadline/cancellation errors during fetchBaseSkill will not trigger stale-cache fallback as intended. The test TestFetchBaseSkill_StaleCacheContextDeadlineFallback passes because the fake fetcher preserves the error chain via %w, masking the production bug.
  Remediation: Either (a) add deadline exceeded and context canceled to gitfetch.transientPatterns so wrapTransient catches them after redaction, or (b) have redactToken preserve the error chain using a wrapper type whose Unwrap() returns the original error.

• [stale-implementation-reference] docs/plans/universal-harness-access.md — Section 4a is renamed to "Git-Based Skill Directory Fetching" and the opening description is updated, but lines immediately following still reference internal/forge/github/directory.go (new) and "GitHub implementation using the Contents API." This creates a confusing section that describes git-based fetching in its header but retains forge-API implementation details.
  Remediation: Update or remove the stale reference to directory.go and Contents API in section 4a, or restructure the section to separate skill fetching (gitfetch) from harness discovery (forge.Client).

• [stale-api-reference] docs/plans/universal-harness-access-phase4.md:51 — Still references "Forge API integration for skill directory fetching (reuses Phase 1 forge client)." The PR updates other lines in this file (lines 29, 40) to reference git sparse checkout but misses line 51.
  Remediation: Update line 51 to reference gitfetch.FetchTree / git sparse checkout, consistent with the other updates in this file.

Previous run (12)

Review

Findings

Medium

• [protected-path] action.yml — Protected path modified. The change reorders the GH_TOKEN env var declaration (moves it from before AGENT to after TARGET_REPO); the value and behavior are unchanged. Human approval is always required for protected-path changes, regardless of the change's scope.

• [stale-api-reference] docs/plans/universal-harness-access-phase2.md — Multiple references to "forge API" for skill directory fetching remain in the phase 2 implementation plan (lines 247, 353, 356). The PR updates the equivalent references in the main plan, phase 1, and phase 4 documents, but misses phase 2. This creates inconsistency across the implementation plan series.
  Remediation: Update references from "forge API" to "git sparse checkout (gitfetch.FetchTree)" in the phase 2 plan, consistent with the updates already made to the other plan documents.

Previous run (13)

Review

Findings

Medium

• [secrets-exposure] internal/gitfetch/gitfetch.go:89 — Token is written to .git/config inside the temp directory via git config http.extraHeader "Authorization: Bearer <tok... The temp directory is cleaned via defer os.RemoveAll, but if the process is killed (SIGKILL) or crashes before cleanup, the config file persists on disk with the credential. Additionally, validateTokenchecks for\rand\nbut does not reject NUL bytes or other control characters (bytes < 0x20 except tab) that could interfere with git config parsing. Remediation: Consider usingGIT_CONFIG_COUNT/GIT_CONFIG_KEY_0/GIT_CONFIG_VALUE_0environment variables (git 2.31+) to pass the auth header without writing it to disk. ExtendvalidateToken` to reject NUL bytes and other control characters.

• [forge-abstraction-violation] internal/harness/compose.go — The change replaces forge.Client calls (ListDirectoryContents + GetFileContentAtRef) with gitfetch.FetchTree, which uses generic git commands rather than forge-specific APIs. ADR-0005 states "All forge operations go through the forge.Client interface," but gitfetch uses forge-neutral git protocol — not forge-specific packages — so the spirit of ADR-0005 is arguably preserved. However, ADR-0005 itself was not updated to document this exception or clarify that git-protocol operations are outside its scope.
  Remediation: Add an annotation to ADR-0005 noting that git-protocol operations (used for skill directory fetching) are intentionally outside the forge.Client abstraction, since they are forge-agnostic by design.

• [interface-evolution] internal/resolve/resolve.go:39 — ResolveOpts, ComposeOpts, and ServiceConfig structs change from ForgeClient forge.Client to TreeFetcher gitfetch.TreeFetchFunc + GitToken string. This is an internal API change across multiple packages (resolve, harness/compose, fetchsvc, cli) that is the core intent of the PR and is authorized by issue refactor(forge): replace GitHub Contents API with git-based directory fetching #2735. All call sites and tests are updated consistently.

Previous run (14)

Review

Findings

High

• [error-handling] internal/gitfetch/gitfetch.go — redactToken destroys the error chain by using fmt.Errorf("%s", msg). In FetchTree, wrapTransient is called before redactToken (e.g., redactToken(wrapTransient(...), token)). When token is non-empty, the TransientError wrapping is lost because fmt.Errorf("%s", msg) creates a plain *errors.errorString with no Unwrap() method. Downstream, isTransientFetchError in compose.go uses errors.As(err, &transient) to detect gitfetch.TransientError and trigger stale cache fallback. For authenticated fetches (private repos where token is non-empty), transient network errors will never be recognized as transient, so the stale cache fallback will never activate — the error will propagate instead of gracefully serving cached content.
  Remediation: Preserve the error chain in redactToken. One approach: define a redactedError wrapper type whose Unwrap() returns the original error and whose Error() performs the string replacement. Another: reorder the wrapping so redactToken runs first on the inner error message, then wrapTransient wraps the already-redacted error in TransientError.

Previous run (15)

Review

Findings

Medium

• [fail-open] internal/harness/compose.go — isTransientFetchError uses string matching on error messages ("connection refused", "no such host", etc.) to classify transient vs non-transient fetch errors for the stale-cache fallback. While errors.Is is correctly used for context.DeadlineExceeded and context.Canceled, the remaining cases match against git stderr text which is locale-dependent and not a stable API. The inline comment acknowledges this limitation. The risk is that a non-transient error whose message happens to contain a matched substring could incorrectly trigger the stale fallback.
  Remediation: Consider wrapping git errors in a typed error (e.g., GitNetworkError) inside runGit/FetchTree based on exit codes or stderr patterns, then use errors.Is/errors.As in isTransientFetchError rather than string matching on the final error message.

• [data-exposure] internal/fetchsvc/service.go — The HTTP error response now includes the raw error from the tree fetcher: "failed to fetch skill directory: " + err.Error(). The default FetchTree implementation redacts tokens via redactToken, but TreeFetchFunc is a public type — a custom implementation that doesn't redact could leak credentials in the HTTP response to the sandboxed agent.
  Remediation: Apply defense-in-depth by sanitizing errors at the fetchsvc layer before including them in HTTP responses, rather than relying solely on the TreeFetchFunc implementation to redact.

• [stale-reference] docs/plans/universal-harness-access.md, docs/plans/universal-harness-access-phase4.md, docs/ADRs/0038-universal-harness-access.md — Several documentation references to the old forge API skill-fetching approach (ListDirectoryContents, GetFileContentAtRef) remain stale after this PR. The PR updated some references but missed others:

  • docs/plans/universal-harness-access.md:47 — "skills require forge API access (e.g., GitHub Contents API)"
  • docs/plans/universal-harness-access.md:383 — "runner uses the forge API to list and fetch the skill directory"
  • docs/plans/universal-harness-access-phase4.md:51 — "Forge API integration for skill directory fetching"
  • docs/ADRs/0038-universal-harness-access.md:151 — "resolver uses forge APIs (GitHub Contents API, GitLab equivalent) to list directory contents"
    Remediation: Update these references to reflect the git sparse checkout approach (gitfetch.FetchTree). Note that ListDirectoryContents/GetFileContentAtRef remain in forge.Client for harness discovery (per phase 3 plan) — the updates should clarify that skill directory fetching specifically now uses git sparse checkout.

---

Labels: PR modifies Go source code across harness, forge, and fetch service packages and introduces a new gitfetch package.

Previous run (16)

Review

Findings

Medium

• [behavioral-change] internal/cli/lock.go, internal/cli/run.go — Token resolution failure downgraded from hard error to silent discard. Previously, when h.HasURLSkills() was true and resolveToken() failed, both lockOneAgent and runAgent returned a hard error (Skill URLs require a GitHub token). The new code unconditionally discards the error via resolveGitToken, _ = resolveToken() and proceeds with an empty token. For private repos with URL skills, this delays the failure from a clear "no token" message to an opaque git auth error deep inside FetchTree. The downstream hint in compose.go and resolve.go when opts.GitToken is empty (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) partially mitigates this, but the user sees two separate messages (the initial warning, then the fetch error with a hint) rather than failing fast with a single clear message.
  Remediation: Consider logging a warning (via printer.StepWarn) when resolveToken() fails and h.HasURLSkills() is true, so users get early signal that private repo fetches will fail.

• [test-integrity] internal/fetchsvc/service_test.go — TestHandleFetch_RateLimitRollbackOnFailure creates a Service without setting TreeFetcher, which causes New() to default it to gitfetch.FetchTree. The test expects a failure, but the default FetchTree will execute real git init, git remote add, and git fetch commands against https://github.com/org/repo.git. This makes the test slow (network timeout), potentially flaky, and environment-dependent. Other tests in the same file (e.g., TestHandleFetch_TreeFetcherError) correctly inject a fake TreeFetchFunc.
  Remediation: Inject a failing TreeFetchFunc that returns an immediate error, matching the pattern used in TestHandleFetch_TreeFetcherError.

• [fail-open] internal/harness/compose.go — The stale-cache fallback logic in fetchBaseSkill serves stale cached content when the tree fetcher encounters a "transient" error. The transient-error classification uses strings.ToLower + substring matching ("timeout", "connection refused", "no such host", etc.), which is fragile — Go error messages are not part of a stable API and the approach cannot distinguish a genuine auth failure whose message happens to contain a transient-sounding word. The inversion from the prior code (which only served stale when no ForgeClient was available, not on fetch failure) introduces a new failure mode where network disruption can force stale, potentially incomplete skill content.
  Remediation: Replace the substring-based transient error classification with checking for specific Go error types (e.g., net.Error with Timeout(), context.DeadlineExceeded, syscall.ECONNREFUSED). Consider defaulting to propagating errors and only falling back to stale cache for a narrow, well-defined set of transient errors.

Previous run (17)

Review

Findings

Medium

• [behavioral-change] internal/cli/lock.go, internal/cli/run.go — Token resolution failure downgraded from hard error to warning in both lockOneAgent and runAgent. Previously, when h.HasURLSkills() was true and no token was available, both functions returned a hard error (Skill URLs require a GitHub token). The new code logs a warning via StepWarn and proceeds with an empty token. For public repos this is correct (no token needed for git clone). For private repos, the git fetch will fail later with a potentially opaque authentication error. The downstream hint in compose.go and resolve.go when opts.GitToken is empty (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) partially mitigates this, but the user sees two separate messages (the initial warning, then the fetch error with a hint) rather than failing fast with a single clear message.
  Remediation: Consider preserving the hard-error behavior when h.HasURLSkills() is true and no token can be resolved, or ensure the git fetch error message clearly directs users to set GH_TOKEN/GITHUB_TOKEN.

Previous run (18)

Review

Findings

High

• [logic-error] internal/gitfetch/gitfetch.go — --filter=blob:none on the git fetch command causes authentication to be unavailable during lazy blob fetching for private repositories. The authConfigArgs function produces -c http.extraHeader=Authorization: Bearer *** flags that are only applied to the git fetchcommand. These command-line-coverrides do not persist in.git/config. When git checkout FETCH_HEADtriggers on-demand blob requests to the promisor remote (required because blobs were filtered out), no credentials are available for the connection, causing authentication failures for private repos. The testTestFetchTree_TokenViaExtraHeaderusesfile://protocol which does not require auth, so it does not detect this issue. Public repos work correctly because no auth is needed for the lazy fetch. Remediation: Either (a) remove--filter=blob:nonefrom the fetch args so all blobs for sparse-checkout paths are downloaded during the authenticated fetch, or (b) persist the auth config in the temp repo before fetching viarunGit(ctx, tmpDir, "config", "http.extraHeader", "Authorization: Bearer "+t... — the temp dir is cleaned up by defer os.RemoveAll(tmpDir), so persisting is safe.

Medium

• [behavioral-change] internal/cli/lock.go, internal/cli/run.go — Token resolution failure downgraded from hard error to warning in both lockOneAgent and runAgent. Previously, when h.HasURLSkills() was true and no token was available, both functions returned a hard error (Skill URLs require a GitHub token). The new code logs a warning via StepWarn and proceeds with an empty token. For public repos this is correct (no token needed for git clone). For private repos, the git fetch will fail later with a potentially opaque authentication error. The hint message in the warning and the downstream hint in compose.go/resolve.go when opts.GitToken is empty (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) partially mitigate this, but the user sees two separate messages (the initial warning, then the fetch error with a hint) rather than failing fast with a single clear message.
  Remediation: Consider whether a hard error for private repos is more appropriate, or ensure the git fetch error message clearly directs users to set GH_TOKEN/GITHUB_TOKEN.

Previous run (19)

Review

Findings

Medium

• [behavioral-change] internal/cli/lock.go:300, internal/cli/run.go:301 — Token resolution failure downgraded from hard error to warning in both lockOneAgent and runAgent. Previously, when h.HasURLSkills() was true and no token was available, both functions returned a hard error (Skill URLs require a GitHub token). The new code logs a warning via StepWarn and proceeds with an empty token. For public repos this is correct (no token needed for git clone). For private repos, the git fetch will fail later with a potentially opaque authentication error. The hint message in the warning and the downstream hint in compose.go/resolve.go when opts.GitToken is empty (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) partially mitigate this, but the user sees two separate messages (the initial warning, then the fetch error with a hint) rather than failing fast with a single clear message.
  Remediation: Consider whether a hard error for private repos is more appropriate, or ensure the git fetch error message clearly directs users to set GH_TOKEN/GITHUB_TOKEN.

Previous run (20)

Review

Findings

Medium

• [behavioral-change] internal/cli/lock.go:300, internal/cli/run.go:212 — Token resolution failure downgraded from hard error to warning. Previously, when h.HasURLSkills() was true and no token was available, both lockOneAgent and runAgent returned a hard error (Skill URLs require a GitHub token). The new code logs a warning via StepWarn and proceeds with an empty token. For public repos this is correct (no token needed for git clone). For private repos, the git fetch will fail later with a potentially opaque authentication error. The downstream hint appended in compose.go and resolve.go when opts.GitToken is empty (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) partially mitigates this, but the user sees two separate messages (the initial warning, then the fetch error with a hint) rather than failing fast with a single clear message.
  Remediation: Consider restoring the fail-fast behavior specifically when URL skills target private repos, or combine the hint into the initial warning message so the user gets actionable guidance before the slow git fetch attempt.

• [secrets-exposure] internal/gitfetch/gitfetch.go:943 — The token is passed to git via -c http.extraHeader=Authorization: Bearer *** as a command-line argument, making it visible in the process table (/proc/*/cmdline) for the duration of the git fetch command. The code comment acknowledges this trade-off. Additionally, authConfigArgs does not validate the token value — a token containing newline characters could inject additional git config values.
  Remediation: Validate that the token does not contain newline or carriage-return characters before interpolating it into the config value.

• [stale-api-reference] docs/plans/adr-0045-forge-portable-harness-phase3.md:11 — Phase 3 implementation plan references ListDirectoryContents and GetFileContentAtRef methods.
  Remediation: Add a clarifying note to the Phase 3 plan distinguishing the harness-discovery use case from the skill-directory fetching use case.

Previous run (21)

Review

Findings

Medium

• [behavioral-change] internal/cli/lock.go:300, internal/cli/run.go:219 — Token resolution failure downgraded from hard error to warning. Previously, when h.HasURLSkills() was true and no token was available, both lockOneAgent and runAgent returned a hard error (Skill URLs require a GitHub token). The new code logs a warning via StepWarn and proceeds with an empty token. For public repos this is correct (no token needed for git clone). For private repos, the git fetch will fail with an opaque authentication error later in the pipeline. The downstream code in compose.go and resolve.go partially mitigates this by appending (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) when opts.GitToken is empty — but the user first sees the vague warning, then the git error, then the hint. The old single-line error was more actionable.
  Remediation: Consider restoring the fail-fast behavior specifically when URL skills target private repos, or promote the hint to the initial warning message.

• [error-handling-gap] internal/harness/compose.go:822 — The stale-cache fallback in fetchBaseSkill can serve incomplete cached data on transient fetch errors. When a stale cache entry has FullListing=false (v0.22.0 format, known to lack companion files like sub-agents and scripts), a transient network error causes the function to return this incomplete directory with a Warning field set on the Dependency. The warning IS surfaced via printResolvedDeps in lock.go, but the skill may silently run without its companion files. Previously, stale entries were only re-fetched when a ForgeClient was available; now they are always re-fetched when online, which increases the chance of hitting this path.
  Remediation: Only fall back to stale cache when entry.FullListing is true (indicating the cache is known-complete), or add explicit logging when serving a stale incomplete entry.

• [fail-open-auth] internal/harness/compose.go:833 — The stale-cache fallback uses substring matching on error messages to distinguish auth failures (which should propagate) from transient errors (which should fall back to cache). The code uses strings.ToLower and checks for "authentication", "403", "401", "could not read username", and "terminal prompts disabled" — a significant improvement over the prior review's case-sensitive matching. However, git error messages are locale-dependent (translated via gettext in non-English locales), so an auth failure in a non-English locale could use a different string and silently serve stale content.
  Remediation: Invert the logic — treat all errors as non-recoverable by default and only fall back to stale cache for a narrow, well-defined set of transient errors (e.g., network timeouts, DNS failures). This is more defensive than trying to enumerate all possible auth-error strings.

Previous run (22)

Review

Findings

Medium

• [fail-open-auth] internal/harness/compose.go:833 — The stale-cache fallback in fetchBaseSkill uses case-sensitive substring matching (strings.Contains(errMsg, "authentication")) to detect auth errors and propagate them instead of falling back to stale cache. However, git's authentication failure message uses uppercase "Authentication" (e.g., fatal: Authentication failed for...), so the check will never match. This means authentication failures will silently fall back to stale cached content instead of propagating the error. The "403" and "401" checks provide partial mitigation only if the git error output includes HTTP status codes, which is not guaranteed.
  Remediation: Use strings.Contains(strings.ToLower(errMsg), "authentication") or match on additional git-specific error strings like "could not read Username", "terminal prompts disabled", or "Authentication failed".

• [token-leakage] internal/gitfetch/gitfetch.go:98 — authenticatedURL() embeds the token in the git clone URL via url.UserPassword("x-access-token", token). This URL is passed as a command argument to git remote add origin <authURL>, making it visible via /proc/<pid>/cmdline on shared runners and written to .git/config on disk. The defer os.RemoveAll(tmpDir) mitigates disk persistence under normal operation, but if the process crashes or is killed before cleanup, credentials persist on disk. The token is also visible in the process table for the duration of all git operations.
  Remediation: Use git -c http.extraHeader='Authorization: Bearer TOKEN' for the fetch command only, which avoids persisting the token in .git/config and reduces process-table exposure to a single command.

Previous run (23)

Review

Findings

High

• [fail-open-auth] internal/cli/run.go:301, internal/cli/lock.go:295 — Token resolution failure is downgraded from a hard error to a non-fatal warning. Previously, when a harness had URL-referenced skills and no token was available, runAgent returned a fatal, actionable error (Skill URLs require a GitHub token). After this PR, the token resolution failure is logged via StepWarn and execution continues with an empty token. For public repos this works, but for private repos the git fetch will fail later with an opaque git authentication error instead of the previous clear diagnostic pointing users to set GH_TOKEN, GITHUB_TOKEN, or run gh auth login. The same pattern applies in lockOneAgent (lock.go).
  Remediation: Restore the fail-fast behavior when URL skills are present but no token is available — at least when HasURLSkills() returns true. Alternatively, wrap the downstream gitfetch.FetchTree error with a hint when gitToken is empty (e.g., "hint: set GH_TOKEN or GITHUB_TOKEN for private repos").

Medium

• [behavioral-change] action.yml:357 — Removing GH_TOKEN: ${{ inputs.github_token }} from the "Run fullsend" step means resolveToken() will no longer find a token via env during the compose and resolve phases (before the mint service sets it). For private repos with URL-based skills that do NOT use the mint service, skill fetching will fail with an opaque git authentication error instead of succeeding.
  Remediation: Either keep GH_TOKEN in the env (it can still be used for git authentication via the new resolveToken path), or document the breaking change for private-repo skill URLs.

• [fail-open-auth] internal/harness/compose.go:822 — fetchBaseSkill now silently falls back to a stale cache entry when fetchBaseSkillDir fails (if err != nil && staleFallback != nil { return *staleFallback ... }). ANY fetch error — including authentication failures and integrity check failures — will silently serve potentially outdated cached content without surfacing the error. In the prior code, fetch errors were always fatal. A stale cache entry (FullListing=false) may lack companion files (sub-agents, scripts).
  Remediation: Emit a warning when falling back to stale cache so callers can surface it. Restrict the fallback to network-related errors only — integrity check failures (hash mismatches) and authentication errors should not fall back to stale content.

• [stale-doc] docs/ADRs/0038-universal-harness-access.md:210 — ADR-0038 states "Forge interface extension (internal/forge/): Add ListDirectoryContents and GetFileContentAtRef to support skill directory listing and file retrieval at specific refs." This PR implements skill fetching via git sparse checkout (gitfetch.FetchTree) instead, making this ADR section incorrect.
  Remediation: Update ADR-0038 "Changes required" item 6 to describe the git-based approach (TreeFetcher / gitfetch.FetchTree).

Previous run (24)

Review

Findings

High

• [fail-open-auth] internal/cli/run.go:301, internal/cli/lock.go:295 — Token resolution failure is downgraded from a hard error to a non-fatal warning. Previously, when a harness had URL-referenced skills and no token was available, runAgent returned a fatal, actionable error (Skill URLs require a GitHub token). After this PR, the token resolution failure is logged via StepWarn and execution continues with an empty token. For public repos this works, but for private repos the git fetch will fail later with an opaque git authentication error instead of the previous clear diagnostic pointing users to set GH_TOKEN, GITHUB_TOKEN, or run gh auth login. The same pattern applies in lockOneAgent (lock.go).
  Remediation: Restore the fail-fast behavior when URL skills are present but no token is available — at least when HasURLSkills() returns true. Alternatively, wrap the downstream gitfetch.FetchTree error with a hint when gitToken is empty (e.g., "hint: set GH_TOKEN or GITHUB_TOKEN for private repos").

Medium

• [behavioral-change] action.yml:357 — Removing GH_TOKEN: ${{ inputs.github_token }} from the "Run fullsend" step means resolveToken() will no longer find a token via env during the compose and resolve phases (before the mint service sets it). For private repos with URL-based skills that do NOT use the mint service, skill fetching will fail with an opaque git authentication error instead of succeeding.
  Remediation: Either keep GH_TOKEN in the env (it can still be used for git authentication via the new resolveToken path), or document the breaking change for private-repo skill URLs.

• [fail-open-auth] internal/harness/compose.go:822 — fetchBaseSkill now silently falls back to a stale cache entry when fetchBaseSkillDir fails (if err != nil && staleFallback != nil { return *staleFallback ... }). ANY fetch error — including authentication failures and integrity check failures — will silently serve potentially outdated cached content without surfacing the error. In the prior code, fetch errors were always fatal. A stale cache entry (FullListing=false) may lack companion files (sub-agents, scripts).
  Remediation: Emit a warning when falling back to stale cache so callers can surface it. Restrict the fallback to network-related errors only — integrity check failures (hash mismatches) and authentication errors should not fall back to stale content.

• [stale-doc] docs/ADRs/0038-universal-harness-access.md:210 — ADR-0038 states "Forge interface extension (internal/forge/): Add ListDirectoryContents and GetFileContentAtRef to support skill directory listing and file retrieval at specific refs." This PR implements skill fetching via git sparse checkout (gitfetch.FetchTree) instead, making this ADR section incorrect.
  Remediation: Update ADR-0038 "Changes required" item 6 to describe the git-based approach (TreeFetcher / gitfetch.FetchTree).

Previous run (25)

Review

Findings

Medium

• [error-handling-gap] internal/cli/lock.go, internal/cli/run.go — The old code checked h.HasURLSkills() and returned a clear, actionable error ("Skill URLs require a GitHub token") when no token was available. The new code silently discards the token resolution error (resolveGitToken, _ = resolveToken()) in both the compose and resolve paths. For private repos, the git clone will fail with an opaque git error instead of a clear diagnostic. This pattern appears in four locations across lock.go and run.go.
  Remediation: Log a warning when resolveToken() fails and URL skills are present. Consider preserving the early-exit error for private repos by checking h.HasURLSkills() or the forge platform before suppressing the error.

• [token-leakage] internal/gitfetch/gitfetch.go — authenticatedURL() embeds the token in the git remote URL via url.UserPassword("x-access-token", token). The URL is passed to git remote add origin <authURL> as a command argument (visible via /proc/<pid>/cmdline on shared runners) and written to .git/config on disk. redactToken() is only applied to the git fetch error return — other git operation errors (git remote add, git checkout) do not redact. If os.RemoveAll(tmpDir) fails (deferred cleanup), credentials persist on disk.
  Remediation: Apply redactToken() to ALL error returns from FetchTree. Consider using git -c http.extraHeader='Authorization: bearer TOKEN' instead of embedding credentials in the URL, which avoids writing them to .git/config.

• [redaction-fragility] internal/gitfetch/gitfetch.go — redactToken() strips the raw token string, but authenticatedURL() URL-encodes it via url.UserPassword(). If a token contains special characters, the URL-encoded form won't match the raw token during redaction. While GitHub tokens are typically alphanumeric (reducing practical risk), this is a defense-in-depth gap.
  Remediation: Redact both the raw token and its URL-encoded form.

• [behavioral-change] internal/harness/compose.go — The stale-cache check changed from !entry.FullListing && opts.ForgeClient != nil && !opts.FetchPolicy.Offline to !entry.FullListing && !opts.FetchPolicy.Offline. Removing the ForgeClient != nil guard means stale entries are always marked for re-fetch when online, even when no token is available. If the re-fetch fails (private repo, no token), the user gets an error instead of the previously-served stale cached content.
  Remediation: Fall back to the stale cache entry when the re-fetch fails, or check token availability before marking the entry stale.

• [stale-doc] docs/plans/universal-harness-access.md — Implementation plan defines ListDirectoryContents and GetFileContentAtRef as new forge.Client methods for skill directory fetching. This PR implements skill fetching via git sparse checkout instead, making these planned API methods obsolete.
  Remediation: Update the forge interface section to reference TreeFetcher / gitfetch.FetchTree.

• [stale-doc] docs/plans/universal-harness-access-phase1.md — ResolveOpts struct definition includes ForgeClient (forge.Client for skill directory resolution) and the skill resolution algorithm describes calling ForgeClient.ListDirectoryContents and ForgeClient.GetFileContentAtRef. Both are replaced by TreeFetcher + GitToken in this PR.
  Remediation: Update struct definition and algorithm description to match the new implementation.

• [stale-doc] docs/plans/universal-harness-access-phase4.md — States "skills are directories, requiring ListDirectoryContents and GetFileContentAtRef". This PR changes the mechanism to git sparse checkout.
  Remediation: Update to reflect git-based fetching.

---

Labels: PR modifies harness composition, forge URL handling, and skill fetching infrastructure

Previous run (26)

Review

Findings

Medium

• [behavioral-change] internal/cli/lock.go:300, internal/cli/run.go:212 — Token resolution failure downgraded from hard error to warning. Previously, when h.HasURLSkills() was true and no token was available, both lockOneAgent and runAgent returned a hard error (Skill URLs require a GitHub token). The new code logs a warning via StepWarn and proceeds with an empty token. For public repos this is correct (no token needed for git clone). For private repos, the git fetch will fail later with a potentially opaque authentication error. The downstream hint appended in compose.go and resolve.go when opts.GitToken is empty (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) partially mitigates this, but the user sees two separate messages (the initial warning, then the fetch error with a hint) rather than failing fast with a single clear message.
  Remediation: Consider restoring the fail-fast behavior specifically when URL skills target private repos, or combine the hint into the initial warning message so the user gets actionable guidance before the slow git fetch attempt.

• [secrets-exposure] internal/gitfetch/gitfetch.go:943 — The token is passed to git via -c http.extraHeader=Authorization: Bearer *** as a command-line argument, making it visible in the process table (/proc/*/cmdline) for the duration of the git fetch command. The code comment acknowledges this trade-off. Additionally, authConfigArgsdoes not validate the token value — a token containing newline characters could inject additional git config values (e.g.,Bearer x\ncore.sshCommand=evil). Remediation: Validate that the token does not contain newline or carriage-return characters before interpolating it into the config value. Consider using GIT_ASKPASS` or a credential helper to pass the token out-of-band.

• [stale-api-reference] docs/plans/adr-0045-forge-portable-harness-phase3.md:11 — Phase 3 implementation plan references ListDirectoryContents and GetFileContentAtRef methods. While these methods remain in forge.Client for DiscoverRemoteAgents (a different use case), the plan's language could confuse future readers who encounter both the git-based skill fetching and the forge-API harness discovery patterns without context on which applies where.
  Remediation: Add a clarifying note to the Phase 3 plan distinguishing the harness-discovery use case (still uses forge.Client) from the skill-directory fetching use case (now uses gitfetch.FetchTree).

Previous run (27)

Review

Findings

Medium

• [behavioral-change] internal/cli/lock.go:300, internal/cli/run.go:219 — Token resolution failure downgraded from hard error to warning. Previously, when h.HasURLSkills() was true and no token was available, both lockOneAgent and runAgent returned a hard error (Skill URLs require a GitHub token). The new code logs a warning via StepWarn and proceeds with an empty token. For public repos this is correct (no token needed for git clone). For private repos, the git fetch will fail with an opaque authentication error later in the pipeline. The downstream code in compose.go and resolve.go partially mitigates this by appending (hint: set GH_TOKEN or GITHUB_TOKEN for private repos) when opts.GitToken is empty — but the user first sees the vague warning, then the git error, then the hint. The old single-line error was more actionable.
  Remediation: Consider restoring the fail-fast behavior specifically when URL skills target private repos, or promote the hint to the initial warning message.

• [error-handling-gap] internal/harness/compose.go:822 — The stale-cache fallback in fetchBaseSkill can serve incomplete cached data on transient fetch errors. When a stale cache entry has FullListing=false (v0.22.0 format, known to lack companion files like sub-agents and scripts), a transient network error causes the function to return this incomplete directory with a Warning field set on the Dependency. The warning IS surfaced via printResolvedDeps in lock.go, but the skill may silently run without its companion files. Previously, stale entries were only re-fetched when a ForgeClient was available; now they are always re-fetched when online, which increases the chance of hitting this path.
  Remediation: Only fall back to stale cache when entry.FullListing is true (indicating the cache is known-complete), or add explicit logging when serving a stale incomplete entry.

• [fail-open-auth] internal/harness/compose.go:833 — The stale-cache fallback uses substring matching on error messages to distinguish auth failures (which should propagate) from transient errors (which should fall back to cache). The code uses strings.ToLower and checks for "authentication", "403", "401", "could not read username", and "terminal prompts disabled" — a significant improvement over the prior review's case-sensitive matching. However, git error messages are locale-dependent (translated via gettext in non-English locales), so an auth failure in a non-English locale could use a different string and silently serve stale content.
  Remediation: Invert the logic — treat all errors as non-recoverable by default and only fall back to stale cache for a narrow, well-defined set of transient errors (e.g., network timeouts, DNS failures). This is more defensive than trying to enumerate all possible auth-error strings.

Previous run (28)

Review

Findings

Medium

• [fail-open-auth] internal/harness/compose.go:833 — The stale-cache fallback in fetchBaseSkill uses case-sensitive substring matching (strings.Contains(errMsg, "authentication")) to detect auth errors and propagate them instead of falling back to stale cache. However, git's authentication failure message uses uppercase "Authentication" (e.g., fatal: Authentication failed for...), so the check will never match. This means authentication failures will silently fall back to stale cached content instead of propagating the error. The "403" and "401" checks provide partial mitigation only if the git error output includes HTTP status codes, which is not guaranteed.
  Remediation: Use strings.Contains(strings.ToLower(errMsg), "authentication") or match on additional git-specific error strings like "could not read Username", "terminal prompts disabled", or "Authentication failed".

• [token-leakage] internal/gitfetch/gitfetch.go:98 — authenticatedURL() embeds the token in the git clone URL via url.UserPassword("x-access-token", token). This URL is passed as a command argument to git remote add origin <authURL>, making it visible via /proc/<pid>/cmdline on shared runners and written to .git/config on disk. The defer os.RemoveAll(tmpDir) mitigates disk persistence under normal operation, but if the process crashes or is killed before cleanup, credentials persist on disk. The token is also visible in the process table for the duration of all git operations.
  Remediation: Use git -c http.extraHeader='Authorization: Bearer TOKEN' for the fetch command only, which avoids persisting the token in .git/config and reduces process-table exposure to a single command.

Previous run (29)

Review

Findings

High

• [fail-open-auth] internal/cli/run.go:301, internal/cli/lock.go:295 — Token resolution failure is downgraded from a hard error to a non-fatal warning. Previously, when a harness had URL-referenced skills and no token was available, runAgent returned a fatal, actionable error (Skill URLs require a GitHub token). After this PR, the token resolution failure is logged via StepWarn and execution continues with an empty token. For public repos this works, but for private repos the git fetch will fail later with an opaque git authentication error instead of the previous clear diagnostic pointing users to set GH_TOKEN, GITHUB_TOKEN, or run gh auth login. The same pattern applies in lockOneAgent (lock.go).
  Remediation: Restore the fail-fast behavior when URL skills are present but no token is available — at least when HasURLSkills() returns true. Alternatively, wrap the downstream gitfetch.FetchTree error with a hint when gitToken is empty (e.g., "hint: set GH_TOKEN or GITHUB_TOKEN for private repos").

Medium

• [behavioral-change] action.yml:357 — Removing GH_TOKEN: ${{ inputs.github_token }} from the "Run fullsend" step means resolveToken() will no longer find a token via env during the compose and resolve phases (before the mint service sets it). For private repos with URL-based skills that do NOT use the mint service, skill fetching will fail with an opaque git authentication error instead of succeeding.
  Remediation: Either keep GH_TOKEN in the env (it can still be used for git authentication via the new resolveToken path), or document the breaking change for private-repo skill URLs.

• [fail-open-auth] internal/harness/compose.go:822 — fetchBaseSkill now silently falls back to a stale cache entry when fetchBaseSkillDir fails (if err != nil && staleFallback != nil { return *staleFallback ... }). ANY fetch error — including authentication failures and integrity check failures — will silently serve potentially outdated cached content without surfacing the error. In the prior code, fetch errors were always fatal. A stale cache entry (FullListing=false) may lack companion files (sub-agents, scripts).
  Remediation: Emit a warning when falling back to stale cache so callers can surface it. Restrict the fallback to network-related errors only — integrity check failures (hash mismatches) and authentication errors should not fall back to stale content.

• [stale-doc] docs/ADRs/0038-universal-harness-access.md:210 — ADR-0038 states "Forge interface extension (internal/forge/): Add ListDirectoryContents and GetFileContentAtRef to support skill directory listing and file retrieval at specific refs." This PR implements skill fetching via git sparse checkout (gitfetch.FetchTree) instead, making this ADR section incorrect.
  Remediation: Update ADR-0038 "Changes required" item 6 to describe the git-based approach (TreeFetcher / gitfetch.FetchTree).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:23 PM UTC · Completed 8:37 PM UTC
Commit: de25216 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:06 PM UTC · Completed 9:21 PM UTC
Commit: 9895660 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:33 PM UTC · Completed 9:50 PM UTC
Commit: 7213182 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 9:56 PM UTC · Completed 10:11 PM UTC
Commit: a5c1e76 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:20 PM UTC · Completed 10:35 PM UTC
Commit: cb9b8a7 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:09 PM UTC · Completed 2:27 PM UTC
Commit: 1a59952 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:09 PM UTC · Completed 10:23 PM UTC
Commit: 9ae3926 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:15 PM UTC · Completed 11:29 PM UTC
Commit: af3cc41 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:35 PM UTC · Completed 12:02 AM UTC
Commit: 9968ea5 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 12:10 AM UTC · Ended 12:12 AM UTC
Commit: e6b1ae3 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 12:16 AM UTC · Ended 12:26 AM UTC
Commit: e6b1ae3 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 12:29 AM UTC · Completed 12:42 AM UTC
Commit: 4da9c85 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:52 AM UTC · Completed 1:04 AM UTC
Commit: fc0d479 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 1:19 AM UTC · Completed 1:28 AM UTC
Commit: fc0d479 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2736 — Replace GitHub Contents API with git sparse checkout

Workflow overview

This was a human-authored PR by ggallen (22 files, +1372/-424) that replaced forge-specific REST API calls with a forge-agnostic gitfetch.FetchTree implementation using git sparse checkout. The PR was created 2026-06-28 and merged 2026-07-01 (~53 hours).

Review quality: Excellent

The review bot delivered high-quality findings across 30 review rounds:

• 4 high-severity findings: fail-open auth downgrade, error chain destruction breaking TransientError detection, token leakage, blob-none auth failure for private repos
• 17 medium-severity findings: command injection (-- separator), secrets exposure via .git/config, stale-cache fallback fragility, test integrity issues, scaffold contract changes
• The author addressed every single finding with code changes and detailed explanations
• ~60% of unique findings were genuinely new discoveries in changed code (not missed on first pass), validating the incremental review model

Human review comparison

• rh-hemartin approved at 06:08 UTC June 29 with no comments — before the bot discovered several important findings (error chain destruction, command injection, secrets exposure via git config)
• ifireball asked valuable design questions the bot did not: "Did you consider Go git libraries?" and questions about the caching/hash mechanism. The bot focused on implementation bugs, not design alternatives

Cost observation

25 force pushes triggered 30 review rounds (29 status comments + 1 sticky consolidated comment). The most-repeated finding (token resolution downgrade) appeared in ~20 of 29 rounds. This is expensive but was driven by the author's rapid iteration cycle.

Proposals: None (existing issues cover all improvement vectors)

All identified improvement areas are already tracked by open issues in this repo:

Area	Key existing issues
Excessive review rounds	#1370, #947, #2358
Incremental finding discovery	#1582, #1552, #1422
Human approval timing	#2099, #1574
Missing design-level review	#1469, #1194, #2325
Review staleness/dismissal	#827, #956, #1331
Label toggling churn	#355, #927, #1551
Finding repetition across rounds	#1552, #1370

The workflow functioned well overall — the bot caught real security and correctness issues that the early human approval missed, and the author was responsive to all feedback.