feat(cli): add fullsend agent subcommand (ADR 0058 Phase 2)

[ggallen]

Summary

• Adds fullsend agent CLI subcommand with add, list, update, remove for managing agent registrations in config (ADR 0058 Phase 2)
• agent add pins URLs to a commit SHA with integrity hash and auto-updates allowed_remote_resources
• agent update re-pins a URL agent to a new commit SHA (explicit or default branch HEAD)
• agent remove cleans up unused allowlist prefixes when the last agent using a prefix is removed
• Adds GetBranchRef to forge.Client interface for resolving branch HEAD SHAs

Test plan

• All existing internal/cli tests pass
• All existing internal/forge tests pass
• Full go build ./... succeeds
• make go-test passes (all packages)
• make go-vet clean
• New tests cover: local path add, URL add with pinned SHA, URL add with hash mismatch, allowlist auto-update, --name flag, duplicate rejection (case-insensitive), path traversal rejection, list empty/populated/hash-stripped, update re-pin with default branch, update with explicit SHA, update rejects local path, update rejects invalid SHA, remove with allowlist cleanup, remove preserves allowlist when other agents share prefix, per-repo config support, helper function unit tests

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

feat(cli): add fullsend agent subcommand for managing agent registrations (ADR 0058 Phase 2)

✨ Enhancement 🕐 40+ Minutes

AI Description

• Adds fullsend agent CLI with add, list, update, remove subcommands for managing agent
 registrations in org and per-repo config files
• agent add pins URL agents to a commit SHA with SHA256 integrity hash and auto-updates
 allowed_remote_resources; local path agents are validated for existence and path traversal
• agent update re-pins a URL agent to a new commit SHA (explicit or resolved from default branch
 HEAD via GetBranchRef)
• agent remove cleans up unused allowed_remote_resources prefixes when the last agent using that
 prefix is removed
• Introduces AgentEntry type with custom YAML unmarshaler (string shorthand or object form),
 DerivedName() helper, and full validation (name uniqueness, integrity hash, allowlist membership,
 path traversal, scheme checks)
• Extracts URL utility functions (IsURL, ParseIntegrityHash, MatchingAllowedPrefixInList,
 NormalizeURLPath) into a new internal/urlutil package to break circular dependency between
 config and harness
• Adds GetBranchRef to the forge.Client interface with GitHub and fake implementations

Diagram

graph TD
    CLI["fullsend CLI"] --> AgentCmd["agent command"]
    AgentCmd --> Add["agent add"]
    AgentCmd --> List["agent list"]
    AgentCmd --> Update["agent update"]
    AgentCmd --> Remove["agent remove"]

    Add --> ConfigPkg["config package"]
    List --> ConfigPkg
    Update --> ConfigPkg
    Remove --> ConfigPkg

    Add --> ForgeIface["forge.Client interface"]
    Update --> ForgeIface
    ForgeIface --> GHLive["github.LiveClient"]
    ForgeIface --> FakeClient["forge.FakeClient"]

    Add --> FetchPkg["fetch package"]
    Update --> FetchPkg

    ConfigPkg --> URLUtil["urlutil package"]
    HarnessPkg["harness package"] --> URLUtil

    subgraph Legend
        direction LR
        _mod["Module/Package"] ~~~ _ext(["External/Interface"])
    end

Loading

High-Level Assessment

The PR's approach is well-suited to the problem. Extracting URL utilities into a dedicated urlutil leaf package is the correct way to share logic between config and harness without circular imports. The cobra subcommand pattern is consistent with the existing CLI structure. Pinning URLs to commit SHAs with SHA256 integrity hashes is the right security model for agent registration. No meaningfully better alternatives were identified.

Files changed (14) +2305 / -106

Enhancement (5) +773 / -12

config.goAdd AgentEntry schema, validation, and helper functions to config +159/-12

Add AgentEntry schema, validation, and helper functions to config

• Introduces 'AgentEntry' struct with custom YAML unmarshaler supporting string shorthand and object form, and rejects legacy role/name/slug entries. Adds 'DerivedName()', 'validateAgentEntries()', 'DefaultAllowedRemoteResources()', 'DefaultAgentEntries()', and seeds 'AllowedRemoteResources' in 'NewPerRepoConfig'. Both 'OrgConfig' and 'PerRepoConfig' gain 'Agents []AgentEntry' and 'AllowedRemoteResources' fields.

internal/config/config.go

agent.goNew 'fullsend agent' CLI subcommand with add/list/update/remove +593/-0

New 'fullsend agent' CLI subcommand with add/list/update/remove

• Implements the 'agent' cobra command tree with four subcommands. 'runAgentAdd' handles local path validation and URL pinning (SHA resolution + integrity hash). 'runAgentUpdate' re-pins a URL agent to a new SHA. 'runAgentRemove' removes an agent and prunes orphaned allowlist prefixes. Includes helpers for allowlist prefix computation, SHA detection in URLs, and forge client construction.

internal/cli/agent.go

root.goRegister 'agent' subcommand on root CLI +1/-0

Register 'agent' subcommand on root CLI

• Adds 'newAgentCmd()' to the root cobra command so 'fullsend agent' is available as a top-level subcommand.

internal/cli/root.go

forge.goAdd GetBranchRef to forge.Client interface +3/-0

Add GetBranchRef to forge.Client interface

• Extends the 'Client' interface with 'GetBranchRef(ctx, owner, repo, branch) (sha, error)' for resolving a branch's HEAD commit SHA, returning 'ErrNotFound' if the branch does not exist.

internal/forge/forge.go

github.goImplement GetBranchRef on GitHub LiveClient +17/-0

Implement GetBranchRef on GitHub LiveClient

• Calls 'GET /repos/{owner}/{repo}/git/ref/heads/{branch}' and decodes the 'object.sha' field to implement 'GetBranchRef' for the live GitHub client.

internal/forge/github/github.go

Refactor (3) +114 / -87

urlutil.goNew shared URL utility package extracted from harness +104/-0

New shared URL utility package extracted from harness

• Creates a new leaf-level 'urlutil' package containing 'IsURL', 'ParseIntegrityHash', 'MatchingAllowedPrefixInList', and 'NormalizeURLPath'. These were previously private functions in 'internal/harness'; moving them here allows 'internal/config' to use them without creating a circular import.

internal/urlutil/urlutil.go

harness.goDelegate URL helpers to urlutil package +6/-49

Delegate URL helpers to urlutil package

• Replaces inline calls to the private 'normalizeURLPath' function with 'urlutil.NormalizeURLPath' in 'ValidateAllowedRemoteResources' and 'MatchingAllowedPrefix'. Removes the now-redundant 'MatchingAllowedPrefixInList' body, delegating to 'urlutil.MatchingAllowedPrefixInList'.

internal/harness/harness.go

url.goDelegate IsURL and ParseIntegrityHash to urlutil +4/-38

Delegate IsURL and ParseIntegrityHash to urlutil

• Replaces the inline implementations of 'IsURL' and 'ParseIntegrityHash' in the harness package with thin wrappers that call the canonical implementations in 'urlutil'.

internal/harness/url.go

Tests (4) +1413 / -4

urlutil_test.goTests for new urlutil package +192/-0

Tests for new urlutil package

• Comprehensive table-driven tests for 'IsURL', 'ParseIntegrityHash', 'MatchingAllowedPrefixInList', and 'NormalizeURLPath', covering edge cases like path traversal, double-encoding, backslashes, and case-insensitive matching.

internal/urlutil/urlutil_test.go

config_test.goExtensive tests for AgentEntry, validation, and config round-trips +602/-4

Extensive tests for AgentEntry, validation, and config round-trips

• Adds ~535 lines of tests covering YAML unmarshal (string/object/mixed/invalid), 'DerivedName', marshal round-trips, all validation error paths (duplicate names, missing hash, non-HTTPS, allowlist mismatch, path traversal, backslash, degenerate names), and 'DefaultAgentEntries'/'DefaultAllowedRemoteResources' helpers. Updates 'TestParseOrgConfig_IgnoresLegacyAgentsBlock' to assert the error is now returned instead of silently ignored.

internal/config/config_test.go

agent_test.goTests for all agent CLI subcommands and helpers +599/-0

Tests for all agent CLI subcommands and helpers

• 599-line test file covering: config loading (org/per-repo/missing), add (local path, name flag, duplicate rejection, case-insensitive collision, path traversal, non-existent path, URL with pinned SHA, hash mismatch, allowlist auto-update, per-repo config), list (empty, populated, hash stripped from display), update (re-pin via default branch, explicit SHA, local path rejection, not-found, invalid SHA), remove (success, allowlist cleanup, allowlist preserved when shared, not-found), and helper unit tests.

internal/cli/agent_test.go

fake.goAdd GetBranchRef to FakeClient for testing +20/-0

Add GetBranchRef to FakeClient for testing

• Adds a 'BranchRefs map[string]string' field and 'GetBranchRef' implementation to 'FakeClient', keyed by 'owner/repo/branch', with error injection support.

internal/forge/fake.go

Documentation (2) +5 / -3

compose.goUpdate OrgAllowlist doc comment to reflect per-repo usage +4/-2

Update OrgAllowlist doc comment to reflect per-repo usage

• Clarifies that 'OrgAllowlist' in 'ComposeOpts' now covers both org-level and per-repo-level 'allowed_remote_resources', and that callers should merge both when available.

internal/harness/compose.go

agent-registration.mdRename AgentName() to DerivedName() in design doc +1/-1

Rename AgentName() to DerivedName() in design doc

• Updates the plan document to reflect the final method name 'DerivedName()' instead of the earlier draft name 'AgentName()'.

docs/plans/agent-registration.md

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 3:32 AM UTC · Ended 4:02 AM UTC
Commit: 104508d · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 51 rules

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

1. Blob URL pinned incorrectly 🐞 Bug ≡ Correctness

Description

pinAgentURL keeps github.com/.../blob/... URLs and only replaces the ref substring, then hashes
the fetched response from that web URL rather than converting to a raw-content URL. This contradicts
the command’s own example input and will produce unusable agent sources/hashes for common GitHub
URLs.

Code

internal/cli/agent.go[R445-474]

+	sha := info.Ref
+	pinnedURL := cleanURL
+	if !commitSHAPattern.MatchString(sha) {
+		if forgeClient == nil {
+			forgeClient, err = buildForgeClient()
+			if err != nil {
+				return "", err
+			}
+		}
+
+		printer.StepStart(fmt.Sprintf("Resolving %s/%s@%s", info.Owner, info.Repo, sha))
+		resolvedSHA, err := forgeClient.GetBranchRef(ctx, info.Owner, info.Repo, sha)
+		if err != nil {
+			repo, repoErr := forgeClient.GetRepo(ctx, info.Owner, info.Repo)
+			if repoErr != nil {
+				return "", fmt.Errorf("resolving ref %q: %w", sha, err)
+			}
+			resolvedSHA, err = forgeClient.GetBranchRef(ctx, info.Owner, info.Repo, repo.DefaultBranch)
+			if err != nil {
+				return "", fmt.Errorf("resolving default branch: %w", err)
+			}
+		}
+		sha = resolvedSHA
+		pinnedURL = strings.Replace(cleanURL, info.Ref, sha, 1)
+		printer.StepDone("Resolved to " + sha[:12])
+	}
+
+	printer.StepStart("Fetching content and computing integrity hash")
+	content, err := fetch.FetchURL(ctx, pinnedURL, fetch.DefaultPolicy)
+	if err != nil {

Relevance

⭐⭐⭐ High

Repo emphasizes correct raw GitHub URL handling; recently added raw parsing/forge fetch fixes (PRs
#2707,#2525).

PR-#2707
PR-#2525

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI explicitly shows a github.com/.../blob/... URL as an example input, but pinAgentURL
preserves the original URL form and only does a substring ref replacement before calling
fetch.FetchURL to compute the integrity hash. The presence of buildRawURL further indicates raw
URL construction was expected but is not used.

internal/cli/agent.go[112-123]
internal/cli/agent.go[437-486]
internal/cli/agent.go[525-527]

Agent prompt

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

### Issue description
`fullsend agent add` documents GitHub `.../blob/...` URLs as valid input, but `pinAgentURL` never converts them to `raw.githubusercontent.com` URLs before fetching and hashing. As a result, the pinned URL remains a GitHub UI URL and the integrity hash is computed over the wrong bytes.

### Issue Context
- `pinAgentURL` parses forge URLs (`forge.ParseForgeURL`) but then sets `pinnedURL := cleanURL` and updates it via `strings.Replace`.
- There is already a `buildRawURL(owner, repo, sha, path)` helper that appears intended for producing raw content URLs.

### Fix Focus Areas
- internal/cli/agent.go[112-123]
- internal/cli/agent.go[437-486]
- internal/cli/agent.go[525-527]

### Proposed fix
- When `parseAgentSourceURL` succeeds via `forge.ParseForgeURL` (github.com tree/blob) or `forge.ParseRawContentURL`, reconstruct the pinned URL as a raw-content URL:
 - `raw := buildRawURL(info.Owner, info.Repo, resolvedSHA, info.Path)`
 - Use `raw` for fetching + hashing, and store `raw + "#sha256=..."`.
- Avoid `strings.Replace(cleanURL, info.Ref, sha, 1)`; instead rebuild the URL from parsed components so ref replacement cannot hit unrelated substrings.
- Ensure allowlist prefix is derived from the stored (raw) URL so `allowed_remote_resources` matches what will actually be fetched at runtime.

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

---

2. Generic URL ref mis-resolved 🐞 Bug ≡ Correctness

Description

Non-GitHub HTTPS URLs fall back to parseGenericURL, but pinAgentURL still resolves non-SHA refs
via a GitHub forge client (buildForgeClient) and GitHub APIs. This will fail or pin incorrectly
for allowed non-GitHub domains when the ref is a branch/tag (e.g., main).

Code

internal/cli/agent.go[R488-523]

+func parseAgentSourceURL(source string) (*forge.ForgeURLInfo, error) {
+	cleanSource, _, _ := urlutil.ParseIntegrityHash(source)
+	info, err := forge.ParseRawContentURL(cleanSource)
+	if err == nil {
+		return info, nil
+	}
+	info, err = forge.ParseForgeURL(cleanSource)
+	if err == nil {
+		return info, nil
+	}
+	return parseGenericURL(cleanSource)
+}
+
+func parseGenericURL(rawURL string) (*forge.ForgeURLInfo, error) {
+	if !urlutil.IsURL(rawURL) {
+		return nil, fmt.Errorf("not a valid HTTPS URL: %s", rawURL)
+	}
+	parts := strings.Split(strings.TrimPrefix(rawURL, "https://"), "/")
+	if len(parts) < 4 {
+		return nil, fmt.Errorf("URL path too short: need at least /{owner}/{repo}/{ref}")
+	}
+	return &forge.ForgeURLInfo{
+		Owner: parts[1],
+		Repo:  parts[2],
+		Ref:   parts[3],
+		Path:  strings.Join(parts[4:], "/"),
+	}, nil
+}
+
+func buildForgeClient() (forge.Client, error) {
+	token, err := resolveToken()
+	if err != nil {
+		return nil, fmt.Errorf("URL agents require a GitHub token: %w", err)
+	}
+	return gh.New(token), nil
+}

Relevance

⭐⭐⭐ High

Team tends to fix URL/forge correctness and fail-fast behavior around URL sources (PR #2707).

PR-#2707

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code path explicitly falls back to parsing any HTTPS URL as {owner, repo, ref} while the ref
resolution code always instantiates a GitHub client and calls GetBranchRef/GetRepo against
GitHub, regardless of the URL host.

internal/cli/agent.go[437-470]
internal/cli/agent.go[488-515]
internal/cli/agent.go[517-523]

Agent prompt

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

### Issue description
`parseAgentSourceURL` accepts any HTTPS URL via `parseGenericURL`, but `pinAgentURL` resolves non-SHA refs using a GitHub-only client and GitHub repository APIs. For non-GitHub hosts, branch/tag refs cannot be resolved this way and the command will behave incorrectly.

### Issue Context
- `parseGenericURL` turns arbitrary HTTPS paths into `{owner, repo, ref, path}`.
- `buildForgeClient` hardcodes GitHub and even errors with “URL agents require a GitHub token”.

### Fix Focus Areas
- internal/cli/agent.go[437-470]
- internal/cli/agent.go[488-515]
- internal/cli/agent.go[517-523]

### Proposed fix
- Detect host/type:
 - If URL is `github.com` or `raw.githubusercontent.com`, allow resolving branch/tag refs via `GetBranchRef`.
 - Otherwise, require `info.Ref` already be a 40-hex commit SHA (fail fast with a clear error like: “non-GitHub URLs must use a pinned commit SHA”).
- Alternatively (larger scope): add forge support for other hosts and route resolution accordingly, but do not silently treat arbitrary hosts as GitHub repos.

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

---

3. buildForgeClient() inside CLI 📘 Rule violation ⌂ Architecture

Description

The new agent CLI code constructs a GitHub-specific forge client internally when forgeClient is
nil, rather than receiving a forge.Client via dependency injection. This weakens the intended
abstraction boundary and makes forge behavior harder to test/mock consistently across call sites.

Code

internal/cli/agent.go[R517-523]

+func buildForgeClient() (forge.Client, error) {
+	token, err := resolveToken()
+	if err != nil {
+		return nil, fmt.Errorf("URL agents require a GitHub token: %w", err)
+	}
+	return gh.New(token), nil
+}

Relevance

⭐⭐ Medium

DI/refactor-only feedback often rejected (PR #1690), but some abstraction fixes accepted; unclear
for this CLI case.

PR-#1690
PR-#2415

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062052 requires forge operations to go through forge.Client and for new call
sites to receive a forge.Client via dependency injection rather than constructing forge-specific
clients internally. The new CLI subcommands pass nil for forgeClient, and the implementation
builds a concrete GitHub client (gh.New(token)) inside buildForgeClient() when forgeClient is
nil.

Rule 1062052: Route all git forge operations through forge.Client
internal/cli/agent.go[125-128]
internal/cli/agent.go[329-334]
internal/cli/agent.go[517-523]

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/cli/agent.go` constructs a concrete GitHub forge client internally (via `gh.New(token)`) when callers pass `nil` for `forgeClient`. The compliance requirement expects forge operations to be routed via the `forge.Client` interface with dependency injection (callers supply the client), rather than having new logic create forge-specific clients inside command implementations.

## Issue Context
The new `agent` subcommands currently call `runAgentAdd/runAgentUpdate` with `forgeClient=nil`, which triggers internal client construction. This couples the CLI implementation to a specific forge backend and makes testing/composition less consistent.

## Fix Focus Areas
- internal/cli/agent.go[125-128]
- internal/cli/agent.go[165-171]
- internal/cli/agent.go[329-334]
- internal/cli/agent.go[448-453]
- internal/cli/agent.go[517-523]

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

---

4. Absolute local paths allowed 🐞 Bug ⚙ Maintainability

Description

Local-path agents accept absolute paths because validation only rejects .., and
filepath.Join(absDir, source) ignores absDir when source is absolute. This allows writing
non-portable configs and bypasses the intended .fullsend-relative scoping for “local path” agents.

Code

internal/cli/agent.go[R579-591]

+func validateLocalPath(absDir, source string) error {
+	for _, seg := range strings.Split(source, "/") {
+		if seg == ".." {
+			return fmt.Errorf("local path must not contain path traversal (..)")
+		}
+	}
+	path := filepath.Join(absDir, source)
+	if _, err := os.Stat(path); err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("local path does not exist: %s", path)
+		}
+		return fmt.Errorf("checking local path: %w", err)
+	}

Relevance

⭐⭐ Medium

No historical evidence on absolute-path rejection; past validations focused on
traversal/backslashes, not absolute-path policy.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

validateLocalPath only checks for .. and then uses filepath.Join(absDir, source) (which
ignores absDir for absolute inputs). Config validation similarly does not reject absolute local
paths, so such entries can be written and pass cfg.validate().

internal/cli/agent.go[579-592]
internal/config/config.go[338-350]

Agent prompt

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

### Issue description
`validateLocalPath` does not reject absolute paths, and `config.validateAgentEntries` also permits absolute local paths. This allows entries like `/etc/passwd` to be registered and persisted, which is non-portable and defeats `.fullsend` directory scoping.

### Issue Context
- `filepath.Join(absDir, source)` drops `absDir` if `source` is absolute.
- Config validation for local paths only checks for backslashes and `..` segments.

### Fix Focus Areas
- internal/cli/agent.go[579-592]
- internal/config/config.go[338-350]

### Proposed fix
- In `validateLocalPath`:
 - Reject `filepath.IsAbs(source)` (and on Windows also consider `filepath.VolumeName(source) != ""`).
 - Clean and verify the resolved path remains within `absDir` using `filepath.Rel` + prefix check.
- Mirror the same rule in `validateAgentEntries` so hand-edited configs are also rejected.
- Consider emitting the stored path normalized as a relative path (e.g., using `filepath.ToSlash(rel)`), to avoid platform-specific separators in config.

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

---

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:32 AM UTC · Completed 4:02 AM UTC
Commit: 79a49f1 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:11 AM UTC · Completed 4:45 AM UTC
Commit: b1fe060 · View workflow run →

[fullsend-ai-review]

Looks good to me

Previous run

Looks good to me

Previous run (2)

Review

Findings

Medium

• [error-wrapping-consistency] internal/cli/agent.go:142 — In loadAgentConfig, when org config parsing succeeds (orgErr == nil) but Dispatch.Platform is empty, and per-repo parsing also fails (prErr != nil), the final error return reports "parsing per-repo config" even though the data was valid YAML for an org config (just lacking a dispatch section). The caller sees a per-repo parse error for what is actually an ambiguous config type.
  Remediation: Return a more descriptive error mentioning both parse attempts, e.g., "config lacks dispatch.platform (not org config) and per-repo parse failed: %w", prErr.

Low

• [missing-authorization] This PR implements a non-trivial feature (639 lines of new CLI code, forge interface extension) but has no linked GitHub issue. The PR references ADR 0058 Phase 2 in its title, providing traceability to the design decision, but explicit issue linkage improves authorization tracking.

• [edge-case] internal/cli/agent.go:529 — When the URL ref is a tag name (e.g., v1.0.0), GetBranchRef queries /git/ref/heads/{tag} which returns 404 (ErrNotFound). The code then falls back to the default branch HEAD, silently discarding the user's intended tag ref. The user sees a StepInfo message about the fallback but the agent is pinned to a potentially different commit than the tag points to.

• [path-injection] internal/forge/github/github.go:1270 — GetBranchRef constructs the API path via fmt.Sprintf("/repos/%s/%s/git/ref/heads/%s", owner, repo, branch) without URL-encoding the branch parameter. Matches the existing pattern used by CreateBranch in the same file. Practical risk is negligible since callers pass branch names from well-formed GitHub URLs or the API's DefaultBranch field.

• [url-validation] internal/cli/agent.go:464 — isGitHubURL uses strings.Contains(rawURL, "github.com/") rather than URL parsing. A crafted URL like https://evil.com/github.com/... would be misclassified. Impact is limited: misclassification causes the code to use buildRawURL (producing a raw.githubusercontent.com URL) or to allow branch resolution via the forge client, but integrity hashing is always applied regardless of classification.

• [internal-interface-evolution] internal/forge/forge.go:294 — The forge.Client interface gains a new GetBranchRef method. Both existing implementations (FakeClient and LiveClient) are updated in this PR. Any out-of-tree implementations would break at compile time, but this is expected for an internal/ interface.

---

Labels: PR was trimmed to CLI agent subcommand and forge interface only; e2e and mint components are no longer in the diff.

Previous run (3)

Review

Findings

Medium

• [error-handling] internal/cli/agent.go:497 — In pinAgentURL, when the first GetBranchRef returns ErrNotFound and the subsequent GetRepo call fails, the error return wraps the original err (the stale ErrNotFound from GetBranchRef) instead of repoErr (the actual GetRepo failure). This causes the caller to receive a misleading "not found" error when the real problem might be a network timeout, auth failure, or rate limit.
  Remediation: Change return "", fmt.Errorf("resolving ref %q: %w", sha, err) to wrap repoErr instead of err.

• [naming-consistency] docs/runtimes.md:47 — This change replaces AgentName() with DerivedName() in a comment describing how agent definition filenames are derived in the sandbox layout. However, the comment refers to the runtime bootstrap context (internal/runtime/bootstrap.go), where AgentName() is the correct method on the Bootstrap interface. DerivedName() is a method on config.AgentEntry, a different type in the config layer. This substitution introduces an incorrect cross-reference.
  Remediation: Revert to AgentName() since the comment describes the runtime behavior, not the config layer.

• [error-wrapping-consistency] internal/cli/agent.go:152 — loadAgentConfig returns errors from both orgErr and prErr with identical format strings ("parsing config: %w"), which does not indicate which parser failed. While the wrapped errors carry distinct messages, the outer context is ambiguous for debugging.
  Remediation: Differentiate: "parsing as org config: %w" vs "parsing as per-repo config: %w".

Low

• [edge-case] internal/cli/agent.go:489 — When the URL ref is a tag (not a branch), GetBranchRef returns ErrNotFound, causing fallback to default branch HEAD. The user sees a StepInfo message but the agent is pinned to a potentially different commit than the tag the user intended.

• [path-injection] internal/forge/github/github.go:1268 — GetBranchRef constructs the API path without URL-encoding the branch parameter. Matches the existing pattern used by CreateBranch in the same file. Practical risk is negligible since GitHub rejects malformed branch names and the returned SHA is validated against commitSHAPattern.

• [url-validation] internal/cli/agent.go:527 — isGitHubURL uses strings.Contains rather than URL parsing. A crafted URL like https://evil.com/github.com/... would match. Impact is limited: parseAgentSourceURL does proper hostname checks first, and content is integrity-hashed.

• [path-traversal] internal/cli/agent.go:689 — validateLocalPath rejects .. segments by splitting on / only. On Windows, a backslash-based path could bypass this check. The code targets Linux-only deployment, so there is no practical risk in the current environment.

• [internal-interface-evolution] internal/forge/forge.go:294 — The forge.Client interface gains a new GetBranchRef method. The interface is in an internal/ package (not exported), so this only affects code within this module. All implementations (FakeClient, LiveClient) are updated in this PR.

Previous run (4)

Review

Findings

Medium

• [error-handling] internal/cli/agent.go:507 — In pinAgentURL, the fallback from the first GetBranchRef call to the default branch is triggered by any error, not just forge.ErrNotFound. A transient network error, HTTP 500, or rate-limit error would silently fall back to the default branch HEAD instead of surfacing the failure, potentially pinning the agent to an unintended commit. See also: [edge-case] finding at this location.
  Remediation: Check forge.IsNotFound(err) before falling back, and return the original error for non-not-found failures.

• [stale-doc] docs/guides/dev/cli-internals.md:7 — The CLI command tree lists all top-level subcommands but does not include the new fullsend agent subcommand added by this PR. Readers consulting this reference will not discover the agent management commands.
  Remediation: Add the fullsend agent command group (add, list, update, remove) to the CLI command tree.

• [stale-doc] docs/cli/README.md:15 — The command groups and additional commands tables do not include fullsend agent. This is a user-facing CLI reference that will be incomplete after merge.
  Remediation: Add a row for fullsend agent to the additional commands table.

Low

• [edge-case] internal/cli/agent.go:507 — When the URL ref is a tag (not a branch), GetBranchRef returns ErrNotFound (it only queries refs/heads/), causing fallback to the default branch HEAD. The user sees a StepInfo message but the agent is pinned to a potentially different commit than the tag points to.

• [path-injection] internal/forge/github/github.go:1268 — GetBranchRef constructs the API path without URL-encoding the branch parameter. Matches the existing pattern used by CreateBranch in the same file. Practical risk is negligible since GitHub rejects malformed branch names and the returned SHA is validated against commitSHAPattern.

• [url-validation] internal/cli/agent.go:483 — isGitHubURL uses strings.Contains rather than URL parsing. A crafted URL like https://evil.com/github.com/... would match. Impact is limited: parseAgentSourceURL does proper hostname checks first, and content is integrity-hashed.

• [stale-doc] docs/guides/getting-started/operations.md:55 — The standalone commands table does not include the new fullsend agent commands.

Previous run (5)

Review

Findings

High

• [protected-path] .github/, AGENTS.md, skills/ — This PR modifies 6 protected files (.github/scripts/openshell-version.sh, .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml, .github/workflows/release.yml, AGENTS.md, skills/e2e-health/SKILL.md) with no linked issue providing authorization for changes to governance and infrastructure files. Human approval is required for all protected-path changes.
  Remediation: Link an authorizing issue that explains why each protected file is being modified, or split protected-path changes into a separate PR with appropriate justification.

Medium

• [http-api-backward-compatibility] internal/mintcore/handler.go:29 — The mintRequest JSON schema adds a new optional target_org field enabling cross-org token minting. While backward compatible at the API level (existing callers without target_org are unaffected), this fundamentally expands the mint service's security contract to allow issuing tokens for foreign orgs, gated by FULLSEND_FOREIGN_<ROLE>_REPOS allowlists on the target org. See also: [privilege-escalation] finding at this location.
  Remediation: Document the cross-org authorization contract in API documentation.

• [privilege-escalation] internal/mintcore/handler.go:234 — The mint handler now allows repos to be omitted, issuing installation-wide tokens granting access to ALL repositories in the target org's installation. Previously repos was required. Any enrolled workflow can request installation-wide tokens by omitting the repos field. The handler logs a WARNING but has no explicit opt-in gate. See also: [http-api-backward-compatibility] finding at this location.
  Remediation: Consider adding an explicit opt-in mechanism (per-org config flag or role permission) controlling whether installation-wide tokens are permitted.

• [scope-creep] This PR's title claims "feat(cli): add fullsend agent subcommand (ADR 0058 Phase 2)" but the 70-file, ~7300-line diff spans multiple distinct concerns: (1) agent CLI subcommand, (2) fullsend admin foreign CLI, (3) e2e test infrastructure refactoring (~1000 lines removed), (4) two new ADRs (0059, 0060), (5) mintcore cross-org authorization, (6) forge.Client extensions, (7) config schema changes. The title and claimed scope do not match.
  Remediation: Split into separate PRs aligned with distinct authorization, or update title/body to accurately reflect the bundled scope.

Low

• [error-handling] e2e/admin/cleanup.go — In listOrgRepoNames, JSON response body is decoded before checking resp.StatusCode. A non-200 response with non-JSON body produces a confusing parse error instead of an HTTP status error.

• [edge-case] internal/cli/agent.go — In pinAgentURL, when GetBranchRef fails for the specified ref, the code silently falls back to the default branch. A branch reference typo would pin content from a different branch without adequate warning.

• [missing-authorization] This PR has no linked GitHub issue. The PR references ADR 0058 Phase 2 and targets the agent-registration-config-schema base branch, suggesting planned work.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29 — References internal/harness/url.go; URL utilities partially moved to internal/urlutil.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase1.md:22 — References internal/harness/url.go in table; URL utilities partially moved to internal/urlutil.

• [stale-doc] docs/plans/universal-harness-access.md:558 — Shows File: internal/harness/url.go; implementation now partially in internal/urlutil.

Previous run (6)

Review

Findings

Medium

• [nil/panic safety] internal/cli/agent.go — In runAgentUpdate and pinAgentURL, the SHA returned by forgeClient.GetBranchRef is sliced with [:12] for display without validating its length. Three locations: newSHA[:12] twice in runAgentUpdate and sha[:12] once in pinAgentURL. If the GitHub API returns an unexpected or empty string, these will panic with index-out-of-range.
  Remediation: Add a length check before slicing, e.g. validate with commitSHAPattern.MatchString() before using the value, consistent with the explicit-SHA validation path.

• [stale-doc] docs/runtimes.md:47 — Documentation references the old method name AgentName() in a comment about sandbox workspace layout ("filename derived from AgentName()"). This method has been renamed to DerivedName() in this PR, but docs/runtimes.md is not updated.
  Remediation: Update line 47 from AgentName() to DerivedName().

Low

• [missing-authorization] This PR implements a non-trivial feature but has no linked issue. The PR references ADR 0058 Phase 2 and targets the agent-registration-config-schema base branch, indicating planned work. Consider linking an authorizing issue for traceability.

• [edge-case] internal/cli/agent.go — In pinAgentURL, when the URL ref is a tag (not a branch), GetBranchRef fails and the fallback silently resolves to the repo's default branch HEAD, pinning to a potentially different commit than intended. Consider logging a warning when falling back.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29 — References internal/harness/url.go as the location for URL utility functions. Some of these have been extracted to internal/urlutil.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase1.md:22 — Table references internal/harness/url.go as a key artifact; URL utilities have been partially moved to internal/urlutil.

• [stale-doc] docs/plans/universal-harness-access.md:558 — Shows implementation code under File: internal/harness/url.go; the implementation is now partially in internal/urlutil.

• [naming-convention] internal/config/config.go:148 — The validAgentName regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) requires an alphanumeric first character, while the validAgentName in harness.go (^[a-zA-Z0-9_-]+$) allows leading hyphens/underscores. The stricter config regex is a reasonable design choice but the discrepancy is worth noting.

• [path-traversal] internal/cli/agent.go:635 — validateLocalPath splits on / to check for .. segments rather than using filepath.Separator. Mitigated by validateAgentEntries in config.go which separately rejects backslashes.

Previous run (7)

Review

Findings

Medium

• [error-handling] internal/cli/agent.go:497 — In pinAgentURL, when the first GetBranchRef returns ErrNotFound and the subsequent GetRepo call fails, the error return wraps the original err (the stale ErrNotFound from GetBranchRef) instead of repoErr (the actual GetRepo failure). This causes the caller to receive a misleading "not found" error when the real problem might be a network timeout, auth failure, or rate limit.
  Remediation: Change return "", fmt.Errorf("resolving ref %q: %w", sha, err) to wrap repoErr instead of err.

• [naming-consistency] docs/runtimes.md:47 — This change replaces AgentName() with DerivedName() in a comment describing how agent definition filenames are derived in the sandbox layout. However, the comment refers to the runtime bootstrap context (internal/runtime/bootstrap.go), where AgentName() is the correct method on the Bootstrap interface. DerivedName() is a method on config.AgentEntry, a different type in the config layer. This substitution introduces an incorrect cross-reference.
  Remediation: Revert to AgentName() since the comment describes the runtime behavior, not the config layer.

• [error-wrapping-consistency] internal/cli/agent.go:152 — loadAgentConfig returns errors from both orgErr and prErr with identical format strings ("parsing config: %w"), which does not indicate which parser failed. While the wrapped errors carry distinct messages, the outer context is ambiguous for debugging.
  Remediation: Differentiate: "parsing as org config: %w" vs "parsing as per-repo config: %w".

Low

• [edge-case] internal/cli/agent.go:489 — When the URL ref is a tag (not a branch), GetBranchRef returns ErrNotFound, causing fallback to default branch HEAD. The user sees a StepInfo message but the agent is pinned to a potentially different commit than the tag the user intended.

• [path-injection] internal/forge/github/github.go:1268 — GetBranchRef constructs the API path without URL-encoding the branch parameter. Matches the existing pattern used by CreateBranch in the same file. Practical risk is negligible since GitHub rejects malformed branch names and the returned SHA is validated against commitSHAPattern.

• [url-validation] internal/cli/agent.go:527 — isGitHubURL uses strings.Contains rather than URL parsing. A crafted URL like https://evil.com/github.com/... would match. Impact is limited: parseAgentSourceURL does proper hostname checks first, and content is integrity-hashed.

• [path-traversal] internal/cli/agent.go:689 — validateLocalPath rejects .. segments by splitting on / only. On Windows, a backslash-based path could bypass this check. The code targets Linux-only deployment, so there is no practical risk in the current environment.

• [internal-interface-evolution] internal/forge/forge.go:294 — The forge.Client interface gains a new GetBranchRef method. The interface is in an internal/ package (not exported), so this only affects code within this module. All implementations (FakeClient, LiveClient) are updated in this PR.

Previous run (8)

Review

Findings

Medium

• [error-handling] internal/cli/agent.go:507 — In pinAgentURL, the fallback from the first GetBranchRef call to the default branch is triggered by any error, not just forge.ErrNotFound. A transient network error, HTTP 500, or rate-limit error would silently fall back to the default branch HEAD instead of surfacing the failure, potentially pinning the agent to an unintended commit. See also: [edge-case] finding at this location.
  Remediation: Check forge.IsNotFound(err) before falling back, and return the original error for non-not-found failures.

• [stale-doc] docs/guides/dev/cli-internals.md:7 — The CLI command tree lists all top-level subcommands but does not include the new fullsend agent subcommand added by this PR. Readers consulting this reference will not discover the agent management commands.
  Remediation: Add the fullsend agent command group (add, list, update, remove) to the CLI command tree.

• [stale-doc] docs/cli/README.md:15 — The command groups and additional commands tables do not include fullsend agent. This is a user-facing CLI reference that will be incomplete after merge.
  Remediation: Add a row for fullsend agent to the additional commands table.

Low

• [edge-case] internal/cli/agent.go:507 — When the URL ref is a tag (not a branch), GetBranchRef returns ErrNotFound (it only queries refs/heads/), causing fallback to the default branch HEAD. The user sees a StepInfo message but the agent is pinned to a potentially different commit than the tag points to.

• [path-injection] internal/forge/github/github.go:1268 — GetBranchRef constructs the API path without URL-encoding the branch parameter. Matches the existing pattern used by CreateBranch in the same file. Practical risk is negligible since GitHub rejects malformed branch names and the returned SHA is validated against commitSHAPattern.

• [url-validation] internal/cli/agent.go:483 — isGitHubURL uses strings.Contains rather than URL parsing. A crafted URL like https://evil.com/github.com/... would match. Impact is limited: parseAgentSourceURL does proper hostname checks first, and content is integrity-hashed.

• [stale-doc] docs/guides/getting-started/operations.md:55 — The standalone commands table does not include the new fullsend agent commands.

Previous run (9)

Review

Findings

Medium

• [error-handling] internal/cli/agent.go:507 — In pinAgentURL, the fallback from the first GetBranchRef call to the default branch is triggered by any error, not just forge.ErrNotFound. A transient network error, HTTP 500, or rate-limit error would silently fall back to the default branch HEAD instead of surfacing the failure, potentially pinning the agent to an unintended commit. See also: [edge-case] finding at this location.
  Remediation: Check forge.IsNotFound(err) before falling back, and return the original error for non-not-found failures.

• [stale-doc] docs/guides/dev/cli-internals.md:7 — The CLI command tree lists all top-level subcommands but does not include the new fullsend agent subcommand added by this PR. Readers consulting this reference will not discover the agent management commands.
  Remediation: Add the fullsend agent command group (add, list, update, remove) to the CLI command tree.

• [stale-doc] docs/cli/README.md:15 — The command groups and additional commands tables do not include fullsend agent. This is a user-facing CLI reference that will be incomplete after merge.
  Remediation: Add a row for fullsend agent to the additional commands table.

Low

• [edge-case] internal/cli/agent.go:507 — When the URL ref is a tag (not a branch), GetBranchRef returns ErrNotFound (it only queries refs/heads/), causing fallback to the default branch HEAD. The user sees a StepInfo message but the agent is pinned to a potentially different commit than the tag points to.

• [path-injection] internal/forge/github/github.go:1268 — GetBranchRef constructs the API path without URL-encoding the branch parameter. Matches the existing pattern used by CreateBranch in the same file. Practical risk is negligible since GitHub rejects malformed branch names and the returned SHA is validated against commitSHAPattern.

• [url-validation] internal/cli/agent.go:483 — isGitHubURL uses strings.Contains rather than URL parsing. A crafted URL like https://evil.com/github.com/... would match. Impact is limited: parseAgentSourceURL does proper hostname checks first, and content is integrity-hashed.

• [stale-doc] docs/guides/getting-started/operations.md:55 — The standalone commands table does not include the new fullsend agent commands.

Previous run (10)

Review

Findings

High

• [protected-path] .github/, AGENTS.md, skills/ — This PR modifies 6 protected files (.github/scripts/openshell-version.sh, .github/workflows/e2e.yml, .github/workflows/pat-cleanup.yml, .github/workflows/release.yml, AGENTS.md, skills/e2e-health/SKILL.md) with no linked issue providing authorization for changes to governance and infrastructure files. Human approval is required for all protected-path changes.
  Remediation: Link an authorizing issue that explains why each protected file is being modified, or split protected-path changes into a separate PR with appropriate justification.

Medium

• [http-api-backward-compatibility] internal/mintcore/handler.go:29 — The mintRequest JSON schema adds a new optional target_org field enabling cross-org token minting. While backward compatible at the API level (existing callers without target_org are unaffected), this fundamentally expands the mint service's security contract to allow issuing tokens for foreign orgs, gated by FULLSEND_FOREIGN_<ROLE>_REPOS allowlists on the target org. See also: [privilege-escalation] finding at this location.
  Remediation: Document the cross-org authorization contract in API documentation.

• [privilege-escalation] internal/mintcore/handler.go:234 — The mint handler now allows repos to be omitted, issuing installation-wide tokens granting access to ALL repositories in the target org's installation. Previously repos was required. Any enrolled workflow can request installation-wide tokens by omitting the repos field. The handler logs a WARNING but has no explicit opt-in gate. See also: [http-api-backward-compatibility] finding at this location.
  Remediation: Consider adding an explicit opt-in mechanism (per-org config flag or role permission) controlling whether installation-wide tokens are permitted.

• [scope-creep] This PR's title claims "feat(cli): add fullsend agent subcommand (ADR 0058 Phase 2)" but the 70-file, ~7300-line diff spans multiple distinct concerns: (1) agent CLI subcommand, (2) fullsend admin foreign CLI, (3) e2e test infrastructure refactoring (~1000 lines removed), (4) two new ADRs (0059, 0060), (5) mintcore cross-org authorization, (6) forge.Client extensions, (7) config schema changes. The title and claimed scope do not match.
  Remediation: Split into separate PRs aligned with distinct authorization, or update title/body to accurately reflect the bundled scope.

Low

• [error-handling] e2e/admin/cleanup.go — In listOrgRepoNames, JSON response body is decoded before checking resp.StatusCode. A non-200 response with non-JSON body produces a confusing parse error instead of an HTTP status error.

• [edge-case] internal/cli/agent.go — In pinAgentURL, when GetBranchRef fails for the specified ref, the code silently falls back to the default branch. A branch reference typo would pin content from a different branch without adequate warning.

• [missing-authorization] This PR has no linked GitHub issue. The PR references ADR 0058 Phase 2 and targets the agent-registration-config-schema base branch, suggesting planned work.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29 — References internal/harness/url.go; URL utilities partially moved to internal/urlutil.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase1.md:22 — References internal/harness/url.go in table; URL utilities partially moved to internal/urlutil.

• [stale-doc] docs/plans/universal-harness-access.md:558 — Shows File: internal/harness/url.go; implementation now partially in internal/urlutil.

---

Labels: PR modifies mint authorization infrastructure, e2e test framework, and CLI agent commands across multiple components

Previous run (11)

Review

Findings

Medium

• [nil/panic safety] internal/cli/agent.go — In runAgentUpdate and pinAgentURL, the SHA returned by forgeClient.GetBranchRef is sliced with [:12] for display without validating its length. Three locations: newSHA[:12] twice in runAgentUpdate and sha[:12] once in pinAgentURL. If the GitHub API returns an unexpected or empty string, these will panic with index-out-of-range.
  Remediation: Add a length check before slicing, e.g. validate with commitSHAPattern.MatchString() before using the value, consistent with the explicit-SHA validation path.

• [stale-doc] docs/runtimes.md:47 — Documentation references the old method name AgentName() in a comment about sandbox workspace layout ("filename derived from AgentName()"). This method has been renamed to DerivedName() in this PR, but docs/runtimes.md is not updated.
  Remediation: Update line 47 from AgentName() to DerivedName().

Low

• [missing-authorization] This PR implements a non-trivial feature but has no linked issue. The PR references ADR 0058 Phase 2 and targets the agent-registration-config-schema base branch, indicating planned work. Consider linking an authorizing issue for traceability.

• [edge-case] internal/cli/agent.go — In pinAgentURL, when the URL ref is a tag (not a branch), GetBranchRef fails and the fallback silently resolves to the repo's default branch HEAD, pinning to a potentially different commit than intended. Consider logging a warning when falling back.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29 — References internal/harness/url.go as the location for URL utility functions. Some of these have been extracted to internal/urlutil.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase1.md:22 — Table references internal/harness/url.go as a key artifact; URL utilities have been partially moved to internal/urlutil.

• [stale-doc] docs/plans/universal-harness-access.md:558 — Shows implementation code under File: internal/harness/url.go; the implementation is now partially in internal/urlutil.

• [naming-convention] internal/config/config.go:148 — The validAgentName regex (^[a-zA-Z0-9][a-zA-Z0-9_-]*$) requires an alphanumeric first character, while the validAgentName in harness.go (^[a-zA-Z0-9_-]+$) allows leading hyphens/underscores. The stricter config regex is a reasonable design choice but the discrepancy is worth noting.

• [path-traversal] internal/cli/agent.go:635 — validateLocalPath splits on / to check for .. segments rather than using filepath.Separator. Mitigated by validateAgentEntries in config.go which separately rejects backslashes.

---

Labels: PR adds a new CLI feature (fullsend agent subcommand) with config schema changes and harness refactoring.

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 11:26 AM UTC · Ended 11:43 AM UTC
Commit: 104508d · View workflow run →

[rh-hemartin]

Careful with the 48 commits

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:26 AM UTC · Completed 11:43 AM UTC
Commit: 2bee78d · View workflow run →

[github-actions]

Site preview

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

Commit: a07f9ffd25a6eea20fa866b76efb18a10b22e9c9

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 12:09 PM UTC · Ended 12:36 PM UTC
Commit: 104508d · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 83.80282% with 69 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/agent.go	88.33%	30 Missing and 17 partials ⚠️
internal/forge/fake.go	0.00%	11 Missing ⚠️
internal/forge/github/github.go	0.00%	11 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:09 PM UTC · Completed 12:36 PM UTC
Commit: 9f3f190 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 5:17 PM UTC · Completed 5:23 PM UTC
Commit: e5970f3 · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 5:33 PM UTC · Completed 5:39 PM UTC
Commit: 104508d · View workflow run →

[ralphbean]

LGTM. Previous finding addressed.

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:42 PM UTC · Completed 8:54 PM UTC
Commit: 1568f1c · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:59 PM UTC · Completed 9:11 PM UTC
Commit: a07f9ff · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 9:25 PM UTC · Completed 9:35 PM UTC
Commit: a07f9ff · View workflow run →

[fullsend-ai-retro]

Retro: PR #2770 — feat(cli): add fullsend agent subcommand

Timeline

• 03:29 UTC — PR opened by ggallen (1885 additions, 9 files, human-authored)
• 03:31–03:35 — Qodo bot found 2 real bugs (blob URL pinning, generic URL mis-resolution); author fixed both promptly
• 04:02–04:45 — Review agent completed initial reviews; approved with medium findings
• 09:03 — rh-hemartin approved
• 11:28 — rh-hemartin flagged "48 commits" issue (PR was based on stale branch)
• 11:43 — Review agent posted findings on internal/mintcore/handler.go — not modified by this PR (base-branch bleed-through). Author had to manually dismiss.
• 13:05 — ralphbean requested changes (transient error fallback, missing tests) — author showed both were already addressed in the squashed commit
• 13:38 — Review agent posted additional findings; error-wrapping-consistency (medium) was addressed by author
• 17:12 — Author rebased onto main and squashed to single commit
• 16:53–17:33 — 3 consecutive review failures after rebase, including one on stale commit 104508d
• 19:24 — ralphbean approved: "LGTM. Previous finding addressed."
• 20:54–21:11 — Review agent approved twice
• 21:21 — PR merged

Findings

18 review bot events for a single PR (3 failures, 5 terminated/cancelled, 2 duplicate approvals). This is a significant token and CI cost. The root causes are well-tracked by existing issues:

• #2747 — Review agent includes base-branch changes in diff analysis, producing false positives. Directly explains the out-of-scope mintcore/handler.go findings.
• #2599 — Add per-PR review budget to cap runaway review dispatches. Would have limited the 18 review events.
• #963 — Skip review dispatch when HEAD SHA was already reviewed and approved. Would have prevented the duplicate approvals.
• #2399 — Remove stale-head re-dispatch. Would have prevented the review failure on already-gone commit 104508d.
• #1418 — Deduplicate review runs on rapid successive pushes.

Review quality was adequate. The review agent's error-wrapping-consistency finding (medium) was genuinely useful and was addressed by the author. The low-severity findings (tag edge case, path injection, URL validation) were informational and non-harmful. The human reviewer's changes-requested finding turned out to already be addressed in the code, so the review agent correctly did not flag it.

No novel proposals. All observed issues map to existing open issues. This PR's pain points (excessive review runs, out-of-scope findings) are symptoms of well-known gaps already being tracked. The existing issues should be prioritized — particularly #2747 and #2599, which would have the highest impact on reducing wasted work in cases like this PR.