feat(resolve): model skills as directories instead of single files

[ggallen]

Summary

• Skills are directories (SKILL.md + scripts/, sub-agents/, assets/), not single files as modeled in ADR-0038
• The resolver now uses forge APIs (ListDirectoryContents/GetFileContentAtRef) to list and fetch skill directory contents, caches reconstructed trees under <hash>/tree/, and rejects non-forge HTTPS URLs for skills
• Adds ForgeURLInfo parser, directory cache (ComputeTreeHash/CachePutDir/CacheGetDir), Type/Files fields to lock entries, and forge URL validation for skill URLs
• Updates ADR-0038, design doc, and all four phase plans to reflect the directory model

Test plan

• All 37 resolver tests rewritten for directory model and passing
• New tests: SkillDirFetchAndCache, SkillDirCacheHit, SkillDirHashMismatch, SkillNonForgeURLRejected, SkillDirOfflineMiss, SkillDirOfflineHit, NilForgeClientWithSkillURL
• Existing transitive dep tests (chain, diamond, cycle, depth/breadth limits) updated for forge-based directory resolution
• Lock file tests updated for agent+policy (no skill URLs in lock integration tests)
• go test ./... — all packages pass
• go vet ./... — clean
• make lint — all hooks pass

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:36 PM UTC · Completed 8:52 PM UTC
Commit: c5cf3d5 · View workflow run →

[github-actions]

Site preview

Preview: https://eb6438eb-site.fullsend-ai.workers.dev

Commit: 826f132352696324290df1e8b6cadb4aa3ee8cde

[fullsend-ai-review]

Review

Findings

Low

• [unbounded-recursion] internal/forge/github/github.go:843 — ListDirectoryContents has internal bounds (maxDirDepth=10, maxDirAPIcalls=100, maxDirFiles=1000) but these are not counted against the resolver's maxResources limit. In theory 50 skills × 100 API calls = 5000 calls, though in practice the allowlist, integrity hashes, and GitHub rate limits make this unlikely.

• [path-traversal] internal/fetch/cache.go — CacheGetDir uses filepath.Walk which follows symlinks. A symlink planted inside tree/ would be followed during integrity re-verification. Practical exploitability is very low: attacker needs cache write access, and the tree hash comparison would fail on tampered content, so no corrupted data is served. The only impact is reading arbitrary file content into memory during hash computation.

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil if SKILL.md does not exist in the cached skill directory, silently skipping transitive dependency resolution. The tree hash integrity check catches file corruption, but a missing SKILL.md in a skill that should have one would go unnoticed.

• [edge-case] internal/forge/github/github.go:880 — In listDirContents, strings.TrimPrefix(e.Path, path+"/") returns the full path unchanged if the GitHub API returns a path without the expected prefix. This would cause incorrect relative paths and a tree hash mismatch (caught by integrity check), producing a confusing error rather than a security issue.

• [error-message-consistency] internal/cli/lock.go — In both lock.go and run.go, printer.StepFail uses 'Skill URLs require' (capital S) while fmt.Errorf uses 'skill URLs require' (lowercase). This follows Go convention (errors lowercase, UI messages capitalized) but may confuse users who see both.

Info

• [missing-authorization] No linked issue for a 24-file PR. The change aligns with accepted ADR-0038, providing architectural rationale.

Previous run

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go:843 — ListDirectoryContents recurses into subdirectories with maxDirDepth=10, maxDirAPIcalls=100, and maxDirFiles=1000 shared counters. While these counters bound total work within a single invocation, up to 100 sequential GitHub API calls can occur within a single ListDirectoryContents call, and these are not counted against the resolver's maxResources limit. A skill directory with many subdirectories could cause significant latency without hitting the resource limit.
  Remediation: Consider counting each ListDirectoryContents invocation against maxResources, or passing a budget callback so the forge method can coordinate with the resolver's resource counter.

• [stale-doc] docs/guides/user/customizing-agents.md — Multiple examples show skills as single .md files (skills/my-custom-validation.md, skills/my-skill.md, skills/my-custom-linting.md) rather than directories containing SKILL.md. The companion doc customizing-with-skills.md correctly describes the directory model. Users following customizing-agents.md would create incorrect skill structures. This is pre-existing staleness exposed by the directory model change, not introduced by this PR.
  Remediation: Update all skill path examples in customizing-agents.md to show the directory structure (e.g., skills/my-skill/SKILL.md).

Low

• [scope-expansion] PR title suggests "model skills as directories" but also implements lock file functionality (Phase 3). The lock file changes (Type/Files fields in DependencyEntry) are tightly coupled to the directory model and cannot easily be separated.

• [forge-client-instantiation] internal/cli/lock.go, internal/cli/run.go — Both files use resolveToken() for GitHub token resolution when HasURLSkills() is true. Verify that the error message mentions all authentication options including gh auth login for users who don't set environment variables.

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Intentional design: if there is no SKILL.md, there are no dependencies to declare. Validation of skill directory contents belongs at a different layer.

• [edge-case] internal/forge/github/github.go:880 — In listDirContents, when path is empty (repository root), the relative path computation uses relPath = e.Path. Behavior depends on the GitHub API returning paths without leading slashes, which is a stable API contract.

• [test-adequacy] internal/cli/lock_test.go — TestRunLock_SkillDirectoryRoundTrip now provides round-trip coverage (generate lock via runLock, then resolveFromLock). Prior finding addressed.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [path-traversal] internal/fetch/cache.go — CacheGetDir uses filepath.Walk to traverse the tree/ directory during integrity re-verification. filepath.Walk follows symlinks by default. An attacker with write access to the cache directory could plant a symlink pointing outside the cache root. Practical exploitability is low due to multiple defense layers (validateCachePath, atomicWrite, CachePutDir path traversal check, and integrity hash verification).

• [architectural-coherence] internal/resolve/resolve.go:77 — ResolveHarness godoc discusses transitive trust but does not reference ADR-0038 where the security model is documented. Adding a cross-reference would help maintainers understand the trust model.

Info

• [missing-authorization] No linked issue for a non-trivial 23-file PR. The change aligns with accepted ADR-0038, providing architectural rationale.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. Matches the established pattern in ParseIntegrityHash. A literal # in a URL path is essentially impossible in practice.

• [open-redirect-via-relative-url] internal/resolve/relurl.go:11 — ResolveRelativeURL passes through absolute URLs without validation. Defense is effective via downstream allowlist check in resolveURL.

Previous run (2)

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go:843 — ListDirectoryContents recurses into subdirectories with maxDirDepth=10 and maxDirAPIcalls=100 shared counter. While the API call counter bounds total calls, up to 100 sequential GitHub API calls can occur within a single ListDirectoryContents invocation, and these are not counted against the resolver's maxResources limit. A malicious or excessively large skill directory could cause significant latency without hitting the resource limit.
  Remediation: Consider counting each ListDirectoryContents invocation against maxResources, or adding a maxFiles limit to bound the total number of file entries returned.

Low

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality (directory model, forge API methods, lock file extensions). Per COMMITS.md, fix is for user-visible bug fixes. This should be feat(resolve): — GoReleaser uses the prefix to categorize release notes.

• [forge-client-instantiation] internal/cli/lock.go, internal/cli/run.go — Both files directly check os.Getenv for GH_TOKEN/GITHUB_TOKEN instead of using the established resolveToken() pattern found in admin.go. Users who authenticate via gh auth login (no env var) will get an error when running fullsend lock or fullsend run with skill URLs. Error message also omits the gh auth login fallback mentioned by the standard pattern.

• [path-traversal] internal/fetch/cache.go:281 — CacheGetDir uses filepath.Walk to traverse the tree/ directory during integrity re-verification. filepath.Walk follows symlinks by default. An attacker with write access to the cache directory could plant a symlink pointing outside the cache root. Practical exploitability is low due to multiple defense layers (validateCachePath, atomicWrite, CachePutDir path traversal check).

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Intentional design: if there is no SKILL.md, there are no dependencies to declare. Validation of skill directory contents belongs at a different layer.

• [edge-case] internal/forge/github/github.go:880 — In listDirContents, when path is empty (repository root), the relative path computation uses relPath = e.Path. Behavior depends on the GitHub API returning paths without leading slashes, which is a stable API contract.

• [missing-migration-path] docs/ADRs/0038-universal-harness-access.md — The ADR updates skills from single files to directories but does not document a migration path for existing harnesses with single-file skill URLs. The code now rejects non-forge skill URLs at validation time with a clear error, and lock file backward compatibility is handled via empty Type defaulting to "file".

• [scope-expansion] PR title suggests "model skills as directories" but also implements lock file functionality (Phase 3). However, the lock file changes are tightly coupled to the directory model (Type/Files fields) and cannot easily be separated.

• [test-adequacy] internal/cli/lock_test.go — No test verifying the round-trip: generating a lock file with a directory skill via runLock and then using resolveFromLock to resolve from that lock file in a single test. Separate tests exist for each direction.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Essentially impossible in practice since URLs are programmatically constructed.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [stale-harness-example] docs/ADRs/0024-harness-definitions.md:233 — The harness schema example shows skills as individual SKILL.md files rather than directories, predating the directory model introduced by this PR.

Info

• [missing-authorization] No linked issue for a non-trivial 22-file PR. The change aligns with accepted ADR-0038, providing architectural rationale.

• [stale-reference-to-skill-format] docs/runtimes.md:12 — The security matrix references SKILL.md as a scanned artifact. This is not misleading — SKILL.md is still the file within a skill directory that gets scanned.

Previous run (3)

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go:843 — ListDirectoryContents recurses into subdirectories with maxDirDepth=10 and maxDirAPIcalls=100 shared counter. While the API call counter bounds total calls, up to 100 sequential GitHub API calls can occur within a single ListDirectoryContents invocation, and these are not counted against the resolver's maxResources limit. A malicious or excessively large skill directory could cause significant latency without hitting the resource limit.
  Remediation: Consider counting each ListDirectoryContents invocation against maxResources, or adding a maxFiles limit to bound the total number of file entries returned.

Low

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality (directory model, forge API methods, lock file extensions). Per COMMITS.md, fix is for user-visible bug fixes. This should be feat(resolve): — GoReleaser uses the prefix to categorize release notes.

• [forge-client-instantiation] internal/cli/lock.go, internal/cli/run.go — Both files directly check os.Getenv for GH_TOKEN/GITHUB_TOKEN instead of using the established resolveToken() pattern found in admin.go. Users who authenticate via gh auth login (no env var) will get an error when running fullsend lock or fullsend run with skill URLs. Error message also omits the gh auth login fallback mentioned by the standard pattern.

• [path-traversal] internal/fetch/cache.go:281 — CacheGetDir uses filepath.Walk to traverse the tree/ directory during integrity re-verification. filepath.Walk follows symlinks by default. An attacker with write access to the cache directory could plant a symlink pointing outside the cache root. Practical exploitability is low due to multiple defense layers (validateCachePath, atomicWrite, CachePutDir path traversal check).

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Intentional design: if there is no SKILL.md, there are no dependencies to declare. Validation of skill directory contents belongs at a different layer.

• [edge-case] internal/forge/github/github.go:880 — In listDirContents, when path is empty (repository root), the relative path computation uses relPath = e.Path. Behavior depends on the GitHub API returning paths without leading slashes, which is a stable API contract.

• [missing-migration-path] docs/ADRs/0038-universal-harness-access.md — The ADR updates skills from single files to directories but does not document a migration path for existing harnesses with single-file skill URLs. The code now rejects non-forge skill URLs at validation time with a clear error, and lock file backward compatibility is handled via empty Type defaulting to "file".

• [scope-expansion] PR title suggests "model skills as directories" but also implements lock file functionality (Phase 3). However, the lock file changes are tightly coupled to the directory model (Type/Files fields) and cannot easily be separated.

• [test-adequacy] internal/cli/lock_test.go — No test verifying the round-trip: generating a lock file with a directory skill via runLock and then using resolveFromLock to resolve from that lock file in a single test. Separate tests exist for each direction.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Essentially impossible in practice since URLs are programmatically constructed.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [stale-harness-example] docs/ADRs/0024-harness-definitions.md:233 — The harness schema example shows skills as individual SKILL.md files rather than directories, predating the directory model introduced by this PR.

Info

• [missing-authorization] No linked issue for a non-trivial 22-file PR. The change aligns with accepted ADR-0038, providing architectural rationale.

• [stale-reference-to-skill-format] docs/runtimes.md:12 — The security matrix references SKILL.md as a scanned artifact. This is not misleading — SKILL.md is still the file within a skill directory that gets scanned.

Previous run (4)

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go — ListDirectoryContents recurses into subdirectories with maxDirDepth=10 per-invocation, but a wide directory tree can trigger excessive GitHub API calls. Each subdirectory triggers a separate API call, and the resolver's maxResources limit counts resolved skills, not API calls within a single ListDirectoryContents invocation. A repo with 100 subdirs each containing 100 files at depth 1 would produce 101 API calls with no counter.
  Remediation: Add a cumulative API call counter or total file count limit to ListDirectoryContents to bound work within a single skill directory fetch.

• [fail-open] internal/resolve/resolve.go — In resolveSkillDirURL, the ForgeClient nil check happens before the CacheGetDir call. If ForgeClient is nil but the skill directory is already cached, the function errors out unnecessarily with "ForgeClient is required" instead of returning the cached result. This means the --offline flag path still requires a GitHub token even when the cache is fully populated.
  Remediation: Move the ForgeClient nil check to after the CacheGetDir call, inside the if !cacheHit block, so a cache hit doesn't require a ForgeClient.

• [edge-case] internal/forge/github/github.go — In ListDirectoryContents, the relative path computation strings.TrimPrefix(e.Path, path+"/") produces incorrect results when path is empty (repository root). ParseForgeURL allows empty Path (e.g., https://github.com/org/repo/tree/abc123), so this code path is reachable.
  Remediation: Guard the relPath computation: if path is empty, use e.Path directly as relPath. Similarly, in resolveSkillDirURL, handle empty forgeInfo.Path when constructing fullPath.

Low

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality: directory-aware skill resolution via forge API, directory cache with tree hashes, Type/Files fields in lock entries, ForgeURLInfo parser, and new files (url.go, url_test.go). Per COMMITS.md, fix is for user-visible bug fixes. This is feat(resolve): — GoReleaser uses the prefix to categorize release notes.

• [missing-authorization] No linked issue for a non-trivial 21-file PR. Filing an issue would document the rationale for this architectural change and provide explicit authorization.

• [portability] internal/fetch/cache.go — CacheGetDir uses filepath.Walk and filepath.Rel which produce backslash-separated paths on Windows, while CachePutDir stores files with forward-slash keys from the forge API. ComputeTreeHash would produce different results on Windows, causing false integrity verification failures.

• [consumer-completeness] internal/cli/lock.go:244 — resolveFromLock correctly defaults empty Type to "file" for backward compatibility, but there is no explicit test covering the scenario of loading a pre-existing lock file without Type/Files fields.

• [test-adequacy] internal/cli/lock_test.go — TestRunLock_SkillDirectoryType tests the lock-generation flow but does not verify that resolveFromLock correctly handles loading a directory-type dependency from the lock file (the reverse path).

• [architectural-coupling] internal/cli/lock.go, internal/cli/run.go — Both files directly instantiate gh.New(token) when HasURLSkills() is true. This follows the pre-existing pattern used by 13+ other call sites across the codebase, but adding a forge client factory would improve forge-agnosticism.

• [interface-expansion] internal/forge/forge.go — forge.Client interface expanded with ListDirectoryContents and GetFileContentAtRef. The interface already has 40+ methods; both LiveClient and FakeClient are updated. Future forge implementations (GitLab, Forgejo) will need to implement these.

• [adr-modification-without-annotation] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being modified to add the skill directory model. The modifications are additive clarifications, not contradictions, but a brief annotation explaining the update would preserve ADR hygiene.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file" which predates the directory model introduced by this PR.

• [path-traversal] internal/fetch/cache.go:281 — CachePutDir has an explicit path traversal check (strings.HasPrefix on cleaned paths) but does not check for symlinks within the tree directory after files are written. Unexploitable in practice since files come from forge API content bytes, not filesystem operations.

• [integrity-verification] internal/fetch/cache.go:332 — CacheGetDir's filepath.Walk follows symlinks by default. A planted symlink pointing outside the cache could be read into hash computation. Risk limited to information disclosure; integrity verification would fail, preventing use of tampered content.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Treats the skill as a leaf node — reasonable default since SKILL.md is the only file that declares transitive deps.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Very narrow edge case.

Previous run (5)

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go — ListDirectoryContents recurses into subdirectories with no depth limit when recursive=true. A malicious repository (within allowed_remote_resources) with deeply nested directory structures could cause excessive API calls or stack overflow. The resolver's maxResources limit counts resolved skills, not API calls within a single ListDirectoryContents invocation.
  Remediation: Add a maxDepth parameter or internal depth counter (e.g., limit 10) to ListDirectoryContents.

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality: directory cache (CachePutDir, CacheGetDir, ComputeTreeHash), forge API extensions (ListDirectoryContents, GetFileContentAtRef), ForgeURLInfo parser, Type/Files fields in lock entries, and new files (url.go, url_test.go, phase3.md, phase4.md). Per COMMITS.md, fix is for user-visible bug fixes. This is feat(resolve): — GoReleaser uses the prefix to categorize release notes.
  Remediation: Change commit prefix to feat(resolve): to accurately reflect that this adds new capability.

• [architectural-coupling] internal/cli/lock.go, internal/cli/run.go — Both files directly instantiate gh.New(token) when HasURLSkills() is true, hardcoding GitHub as the only forge. Per AGENTS.md, forge-specific logic belongs in internal/forge/. Adding GitLab or Forgejo support would require modifying CLI code.
  Remediation: Introduce a forge client factory that selects the appropriate client based on URL hostname or environment configuration.

• [interface-expansion] internal/forge/forge.go:168 — forge.Client interface expanded with two new methods: ListDirectoryContents and GetFileContentAtRef. The package is internal/forge/ (not externally importable in Go) and both LiveClient and FakeClient are already updated, so no code is currently broken. However, future forge implementations (GitLab, Forgejo) will need to implement these methods.
  Remediation: Document the new interface requirements. Consider interface segregation (e.g., forge.DirectoryClient) for forge implementations that don't support directory listing.

Low

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being modified to add the skill directory model throughout its Decision section. The modifications are additive (extending the existing design) rather than contradictory, but a brief annotation explaining the update would preserve ADR hygiene.

• [url-parsing] internal/forge/url.go — ParseForgeURL accepts both tree (directory) and blob (file) URL formats, but skills always expect tree. A blob URL would parse successfully but fail confusingly at ListDirectoryContents.

• [design-consistency] docs/plans/universal-harness-access-phase1.md — Phase 1 plan states "Validates that skill URLs are from supported forges (GitHub, GitLab)" but IsSupportedForge only accepts github.com. GitLab is claimed but not implemented.

• [breaking-change-undocumented] internal/lock/lock.go — DependencyEntry gains Type and Files fields but lock file version stays at 1. Backward compatibility is correctly handled via omitempty + default-to-file, but this schema evolution is undocumented.

• [consumer-completeness] internal/cli/lock.go:244 — resolveFromLock correctly defaults empty Type to "file" for backward compatibility, but there is no explicit test covering the scenario of loading a pre-existing lock file without Type/Files fields.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Very narrow edge case.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compat for pre-directory-model lock files.

• [path-traversal] internal/fetch/cache.go:281 — CachePutDir has an explicit path traversal check (strings.HasPrefix) but does not check for symlinks within the tree directory after files are written. Files come from forge API (content bytes, not filesystem operations) so this is unexploitable in practice.

• [integrity-verification] internal/fetch/cache.go:332 — CacheGetDir's filepath.Walk follows symlinks by default. A planted symlink pointing outside the cache could be read into hash computation. Risk limited to information disclosure during hash computation; integrity verification would fail, preventing use of tampered content.

• [fail-open] internal/resolve/resolve.go:393 — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Treats the skill as a leaf node — reasonable default since SKILL.md is the only file that declares transitive deps.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file" which predates the directory model introduced by this PR.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows skills as individual SKILL.md files rather than directories.

• [stale-skill-model-reference] docs/guides/user/customizing-agents.md — Multiple examples reference skills as .md files rather than directory structures.

Previous run (6)

Review

Findings

Medium

• [behavioral-inconsistency] internal/cli/run.go:222 — run.go silently proceeds with a nil ForgeClient when no GH_TOKEN/GITHUB_TOKEN is set (if token != "" { forgeClient = gh.New(token) } with no else clause), unlike lock.go which returns a hard error with a clear message ("Skill URLs require a GitHub token (set GH_TOKEN or GITHUB_TOKEN)"). When a harness has URL-referenced skills but no token is available, users get a confusing "ForgeClient is required to resolve skill URL" error from deep in resolveSkillDirURL instead of an early diagnostic at the CLI level.
  Remediation: Mirror lock.go's behavior in run.go: when HasURLSkills() is true and no token is found, emit printer.StepFail and return early with a clear message about setting GH_TOKEN or GITHUB_TOKEN.

• [robustness] internal/cli/lock.go:178 — When building lock entries for directory dependencies, CacheGetDir could return a nil dirEntry (e.g., if metadata is missing). The code guards with if dirEntry != nil but silently produces a lock entry with an empty Files manifest, losing per-file integrity metadata that consumers (resolveFromLock, future audit tools) expect for directory-type entries.
  Remediation: Return an error or log a warning when dirEntry is nil for a directory-type dependency. A directory lock entry without a file manifest is not useful for integrity verification.

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being substantially modified to add the skill directory model, forge interface extension, and directory cache. Per AGENTS.md, Accepted ADRs are point-in-time records. The modifications are additive (extending the design to handle directory-based skills), but the process guidance suggests creating a superseding ADR for substantial changes.
  Remediation: Add a clear annotation to ADR-0038 explaining the update, or create ADR-0039 that supersedes it with the directory-based skill model.

Low

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, an empty Type field in a lock entry defaults to "file". Intentional backward-compatibility measure for pre-directory-model lock files, but could cause unexpected behavior with corrupted or future-versioned lock files.

• [path-traversal] internal/fetch/cache.go:281 — CachePutDir has a path traversal check (strings.HasPrefix) but does not check for symlinks within the tree directory itself. Practical risk is low: validateCachePath covers the parent, cache is keyed by tree hash (requiring content prediction), and MkdirAll creates directories fresh.

• [scope-tier-mismatch] PR uses fix commit prefix but introduces substantial new functionality (directory-based skill model, forge API requirements, interface expansion). feat would be more accurate for GoReleaser release note categorization.

• [api-design] internal/forge/github/github.go — ListDirectoryContents recursively calls itself for subdirectories without a depth limit. Bounded indirectly by the resolver's MaxResources limit and GitHub API rate limiting.

• [url-parsing] internal/forge/url.go — ParseForgeURL accepts both tree (directory) and blob (file) URL formats, but skills always expect tree. A blob URL would pass validation but fail confusingly at ListDirectoryContents.

• [test-coverage] internal/cli/lock_test.go — TestRunLock_GeneratesLockFile no longer tests skills. A new TestRunLock_SkillDirectoryType provides mixed-type coverage (file agent + directory skill).

• [documentation] docs/plans/universal-harness-access.md — Plan doc code example for CachePutDir uses CacheEntry (single-file metadata struct) while actual implementation uses DirCacheEntry with Files field.

• [integrity-verification] internal/fetch/cache.go:332 — CacheGetDir's filepath.Walk follows symlinks by default; a symlink pointing outside the cache could be read into hash computation. Mitigated by validateCachePath on the parent dir and hash comparison.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file" which predates the directory model.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows skills as individual SKILL.md files rather than directories.

• [stale-skill-model-reference] docs/guides/user/customizing-agents.md — Multiple examples (lines 215, 227, 294) reference skills as .md files rather than directory structures.

• [stale-skill-model-reference] docs/agents/triage.md:60 — References creating skills as file paths without clarifying directory model.

Info

• [missing-authorization] No linked issue for a non-trivial 21-file PR. Filing an issue would document the rationale for this architectural change.

Previous run (7)

Review

Findings

Critical

• [error-handling-gap] internal/cli/lock.go:135 — runLock calls resolve.ResolveHarness without setting ForgeClient in ResolveOpts. The diff adds ForgeClient as a required field for skill directory resolution (resolveSkillDirURL checks opts.ForgeClient == nil and returns an error), but neither runLock nor runRun (run.go) are updated to provide a ForgeClient. Any harness with URL-referenced skills will fail at runtime with "ForgeClient is required to resolve skill URL". The lock_test.go was simultaneously changed to remove skill URL testing (replacing it with policy URL testing), masking this regression.
  Remediation: Pass a constructed forge.Client (e.g., github.NewLiveClient) as ForgeClient in the ResolveOpts at both lock.go and run.go. Restore or add a lock_test.go test that exercises the ForgeClient path.

Medium

• [test-integrity] internal/cli/lock_test.go:73 — TestRunLock_GeneratesLockFile was modified to replace skill URL testing with policy URL testing. No test in lock_test.go verifies that runLock correctly populates Type: "directory" and Files manifest for skill directory dependencies via the runLock integration path. The same pattern repeats in TestRunLock_AlreadyUpToDate and TestRunLock_UpdateForceReResolve. While resolve_test.go has comprehensive skill directory tests, the integration path through runLock is untested.
  Remediation: Add a lock_test.go test that uses forge.FakeClient to verify runLock resolves skill directory URLs, populates Type and Files in the lock entry, and passes ForgeClient through to ResolveHarness.

• [path-traversal] internal/forge/github/github.go — GetFileContentAtRef and ListDirectoryContents do not URL-path-escape the path parameter in the API URL. owner and repo use url.PathEscape but path does not. The code checks for ? and # characters but does not escape path segments. While the actual risk is mitigated by paths originating from ParseForgeURL (structured input) and downstream CachePutDir path traversal checks, the inconsistency with how owner and repo are handled is a defense-in-depth gap.
  Remediation: Escape each segment of path individually using url.PathEscape (split on /, escape each segment, rejoin), consistent with how owner and repo are handled.

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being substantially modified to add the skill directory model, forge interface extension, and directory cache. Per AGENTS.md: "Once an ADR on main has status Accepted, it is a point-in-time record. Do not substantially rewrite its Context, Decision, or Consequences sections."
  Remediation: Create ADR-0039 that supersedes ADR-0038 with the directory-based skill model, or add a "Status Note" section explaining why inline modification is appropriate here.

• [interface-expansion-breaking] internal/forge/forge.go:168 — forge.Client interface expanded with two new methods: ListDirectoryContents and GetFileContentAtRef. While this is an internal/ package (not externally importable), it affects all future forge implementations (GitLab, Forgejo) which will need to implement these methods.
  Remediation: Document the new interface requirements. Consider whether interface segregation would be cleaner for forge implementations that don't support directory listing.

Low

• [error-handling-gap] internal/cli/lock.go:155 — In runLock, when dep.Type == "directory", a CacheGetDir miss (nil dirEntry) silently produces a lock entry with no Files manifest. Since the directory was just fetched and cached moments before, a nil dirEntry indicates a logic bug.

• [fail-open] internal/cli/lock.go:238 — In resolveFromLock, when lockDep.Type is empty, it defaults to "file". Intentional backward compatibility for pre-directory-model lock files.

• [inconsistency] docs/plans/universal-harness-access.md:924 — Plan doc code example for CachePutDir uses CacheEntry (single-file metadata struct) while actual implementation uses DirCacheEntry with Files field.

• [path-injection] internal/forge/github/github.go — ListDirectoryContents uses TrimPrefix to compute relative paths. If API returns a path not matching the expected prefix, TrimPrefix returns the string unchanged. Defense-in-depth is present via CachePutDir's path traversal check.

• [design-coherence] internal/forge/url.go — ParseForgeURL accepts both tree (directory) and blob (file) URL formats, but skills always expect tree. A blob URL would parse successfully then fail at ListDirectoryContents with a potentially confusing error.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file (optionally with a scripts/ directory)" which predates the directory model.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows old SKILL.md format, doesn't reflect the directory tree model.

• [schema-evolution-backward-compatible-read] internal/lock/lock.go:38 — Lock file schema evolves without version bump. New Type/Files fields use omitempty, maintaining backward compatibility.

• [architecture-alignment] internal/harness/harness.go — ValidateResourceTypes enforces forge-only for skill URLs. The ADR documents this asymmetry but could be more prominent in the resource types table.

Info

• [rate-limiting] internal/forge/github/github.go — ListDirectoryContents recursively calls itself for subdirectories with no depth limit or max entry count. Bounded indirectly by the resolver's MaxResources limit.

• [missing-authorization] No linked issue for a non-trivial PR (22 files changed). Filing an issue would document the rationale.

• [scope-tier-mismatch] PR uses fix commit prefix but introduces substantial new functionality (forge API methods, directory caching, lock file schema extensions). feat would be more accurate for GoReleaser categorization.

Previous run

Review

Findings

High

• [path-traversal] internal/fetch/cache.go — CachePutDir writes files using filepath.Join(treeDir, relPath) where relPath originates from forge API directory listings (attacker-controlled for public repos). A relative path containing ../ segments would escape the tree/ directory. While validateCachePath is called on the top-level dir, it is NOT called on each individual file path within the tree. The existing CachePut does not have this issue because it writes a single content file with a fixed name. The attack vector starts in resolveSkillDirURL (resolve.go) which trusts forge API response paths without validation.
  Remediation: After computing fullPath := filepath.Join(treeDir, relPath), verify that the cleaned path is still under treeDir before writing. For example: if !strings.HasPrefix(filepath.Clean(fullPath), filepath.Clean(treeDir)+string(filepath.Separator)) { return "", fmt.Errorf("path traversal in file path: %s", relPath) }. Also validate relPath in resolveSkillDirURL before adding to the files map.

Medium

• [path-injection] internal/forge/github/github.go — GetFileContentAtRef and ListDirectoryContents do not URL-path-escape the path parameter in the API URL. owner and repo use url.PathEscape but path does not. If a file path contains ?, #, or spaces, the API call will be malformed. The path value originates from ParseForgeURL and from directory listing entries (e.Path), both of which could contain special characters.
  Remediation: Escape each path segment individually, or at minimum validate that path does not contain query-string-significant characters.

• [error-handling-gap] internal/cli/lock.go — In runLock, when dep.Type == "directory", the code calls fetch.CacheGetDir and silently ignores errors (if err == nil && dirEntry != nil). If CacheGetDir returns an error (e.g., integrity check failure), the lock file entry is written without the files manifest, undermining the reproducibility guarantees that lock files are supposed to provide.
  Remediation: Propagate the error from CacheGetDir or at minimum log a warning that the file manifest could not be populated.

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being substantially modified to change the skills model from single files to directories. Per AGENTS.md: "Once an ADR on main has status Accepted, it is a point-in-time record. Do not substantially rewrite its Context, Decision, or Consequences sections." The diff shows changes to the Decision section adding the skill directory model and forge API requirements.
  Remediation: Clarify whether the original ADR was ambiguous or incorrect. If this is a correction to an architectural mistake, acknowledge it explicitly and justify inline modification vs writing a new ADR.

• [architectural-boundary-violation] internal/harness/forgeurl.go — ParseForgeURL and IsSupportedForge contain forge-specific logic (hardcoded github.com) in the harness package. Per AGENTS.md, forge-specific logic belongs in internal/forge/. When GitLab support is added, this code will need to be updated outside the forge abstraction layer.
  Remediation: Move ParseForgeURL and IsSupportedForge to internal/forge/url.go to keep forge-specific URL format knowledge in the forge abstraction layer.

Low

• [fail-open] internal/cli/lock.go — In resolveFromLock, when lockDep.Type is empty, it defaults to "file". A directory dependency incorrectly recorded without a type will be resolved as a single file, bypassing directory cache integrity checks. Low risk since runLock always sets Type from dep.Type.

• [missing-authorization] No linked issue for a non-trivial PR (20 files, 1796 additions). Filing an issue would document the rationale for this change.

• [scope-tier-mismatch] The PR uses the fix commit prefix but introduces substantial new functionality (forge API methods, directory caching, lock file schema extensions). The fix prefix will cause GoReleaser to categorize this as a bug fix in release notes. Consider feat instead.

• [test-integrity] internal/cli/lock_test.go — TestRunLock_GeneratesLockFile was modified to replace skill URL testing with policy URL testing. No test in lock_test.go verifies that runLock correctly populates Type: "directory" and Files manifest for skill directory dependencies.

• [edge-case] internal/fetch/cache.go — ComputeTreeHash with an empty files map produces a hash of "\n". CachePutDir does not reject empty file maps. Skills are required to have at least SKILL.md.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows skills/triage-coordination/SKILL.md format. Consider updating to reflect the directory model.

• [stale-skill-model-reference] docs/glossary.md:138 — Defines a skill as "A markdown file (optionally with a scripts/ directory)" which predates the current directory model.

• [incomplete-design-documentation] docs/plans/universal-harness-access-phase1.md:123 — States "supported forges (GitHub, GitLab)" but IsSupportedForge only accepts GitHub.

• [relative-url-edge-case] internal/resolve/resolve.go — If a parent URL has a trailing slash, RFC 3986 resolution would append instead of replace sibling. Low risk since ParseForgeURL doesn't produce trailing slashes.

Info

• [rate-limiting] internal/forge/github/github.go — ListDirectoryContents recursively calls itself for subdirectories when recursive=true with no depth limit or max entry count.

• [dependency-introduction] internal/resolve/resolve.go:37 — ResolveOpts gains a ForgeClient field. The code correctly checks for nil and returns a clear error message when skills require it.

Previous run (8)

Review

Findings

Critical

• [error-handling-gap] internal/cli/lock.go:135 — runLock calls resolve.ResolveHarness without setting ForgeClient in ResolveOpts. The diff adds ForgeClient as a required field for skill directory resolution (resolveSkillDirURL checks opts.ForgeClient == nil and returns an error), but neither runLock nor runRun (run.go) are updated to provide a ForgeClient. Any harness with URL-referenced skills will fail at runtime with "ForgeClient is required to resolve skill URL". The lock_test.go was simultaneously changed to remove skill URL testing (replacing it with policy URL testing), masking this regression.
  Remediation: Pass a constructed forge.Client (e.g., github.NewLiveClient) as ForgeClient in the ResolveOpts at both lock.go and run.go. Restore or add a lock_test.go test that exercises the ForgeClient path.

Medium

• [test-integrity] internal/cli/lock_test.go:73 — TestRunLock_GeneratesLockFile was modified to replace skill URL testing with policy URL testing. No test in lock_test.go verifies that runLock correctly populates Type: "directory" and Files manifest for skill directory dependencies via the runLock integration path. The same pattern repeats in TestRunLock_AlreadyUpToDate and TestRunLock_UpdateForceReResolve. While resolve_test.go has comprehensive skill directory tests, the integration path through runLock is untested.
  Remediation: Add a lock_test.go test that uses forge.FakeClient to verify runLock resolves skill directory URLs, populates Type and Files in the lock entry, and passes ForgeClient through to ResolveHarness.

• [path-traversal] internal/forge/github/github.go — GetFileContentAtRef and ListDirectoryContents do not URL-path-escape the path parameter in the API URL. owner and repo use url.PathEscape but path does not. The code checks for ? and # characters but does not escape path segments. While the actual risk is mitigated by paths originating from ParseForgeURL (structured input) and downstream CachePutDir path traversal checks, the inconsistency with how owner and repo are handled is a defense-in-depth gap.
  Remediation: Escape each segment of path individually using url.PathEscape (split on /, escape each segment, rejoin), consistent with how owner and repo are handled.

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being substantially modified to add the skill directory model, forge interface extension, and directory cache. Per AGENTS.md: "Once an ADR on main has status Accepted, it is a point-in-time record. Do not substantially rewrite its Context, Decision, or Consequences sections."
  Remediation: Create ADR-0039 that supersedes ADR-0038 with the directory-based skill model, or add a "Status Note" section explaining why inline modification is appropriate here.

• [interface-expansion-breaking] internal/forge/forge.go:168 — forge.Client interface expanded with two new methods: ListDirectoryContents and GetFileContentAtRef. While this is an internal/ package (not externally importable), it affects all future forge implementations (GitLab, Forgejo) which will need to implement these methods.
  Remediation: Document the new interface requirements. Consider whether interface segregation would be cleaner for forge implementations that don't support directory listing.

Low

• [error-handling-gap] internal/cli/lock.go:155 — In runLock, when dep.Type == "directory", a CacheGetDir miss (nil dirEntry) silently produces a lock entry with no Files manifest. Since the directory was just fetched and cached moments before, a nil dirEntry indicates a logic bug.

• [fail-open] internal/cli/lock.go:238 — In resolveFromLock, when lockDep.Type is empty, it defaults to "file". Intentional backward compatibility for pre-directory-model lock files.

• [inconsistency] docs/plans/universal-harness-access.md:924 — Plan doc code example for CachePutDir uses CacheEntry (single-file metadata struct) while actual implementation uses DirCacheEntry with Files field.

• [path-injection] internal/forge/github/github.go — ListDirectoryContents uses TrimPrefix to compute relative paths. If API returns a path not matching the expected prefix, TrimPrefix returns the string unchanged. Defense-in-depth is present via CachePutDir's path traversal check.

• [design-coherence] internal/forge/url.go — ParseForgeURL accepts both tree (directory) and blob (file) URL formats, but skills always expect tree. A blob URL would parse successfully then fail at ListDirectoryContents with a potentially confusing error.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file (optionally with a scripts/ directory)" which predates the directory model.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows old SKILL.md format, doesn't reflect the directory tree model.

• [schema-evolution-backward-compatible-read] internal/lock/lock.go:38 — Lock file schema evolves without version bump. New Type/Files fields use omitempty, maintaining backward compatibility.

• [architecture-alignment] internal/harness/harness.go — ValidateResourceTypes enforces forge-only for skill URLs. The ADR documents this asymmetry but could be more prominent in the resource types table.

Info

• [rate-limiting] internal/forge/github/github.go — ListDirectoryContents recursively calls itself for subdirectories with no depth limit or max entry count. Bounded indirectly by the resolver's MaxResources limit.

• [missing-authorization] No linked issue for a non-trivial PR (22 files changed). Filing an issue would document the rationale.

• [scope-tier-mismatch] PR uses fix commit prefix but introduces substantial new functionality (forge API methods, directory caching, lock file schema extensions). feat would be more accurate for GoReleaser categorization.

Previous run (9)

Review

Findings

High

• [path-traversal] internal/fetch/cache.go — CachePutDir writes files using filepath.Join(treeDir, relPath) where relPath originates from forge API directory listings (attacker-controlled for public repos). A relative path containing ../ segments would escape the tree/ directory. While validateCachePath is called on the top-level dir, it is NOT called on each individual file path within the tree. The existing CachePut does not have this issue because it writes a single content file with a fixed name. The attack vector starts in resolveSkillDirURL (resolve.go) which trusts forge API response paths without validation.
  Remediation: After computing fullPath := filepath.Join(treeDir, relPath), verify that the cleaned path is still under treeDir before writing. For example: if !strings.HasPrefix(filepath.Clean(fullPath), filepath.Clean(treeDir)+string(filepath.Separator)) { return "", fmt.Errorf("path traversal in file path: %s", relPath) }. Also validate relPath in resolveSkillDirURL before adding to the files map.

Medium

• [path-injection] internal/forge/github/github.go — GetFileContentAtRef and ListDirectoryContents do not URL-path-escape the path parameter in the API URL. owner and repo use url.PathEscape but path does not. If a file path contains ?, #, or spaces, the API call will be malformed. The path value originates from ParseForgeURL and from directory listing entries (e.Path), both of which could contain special characters.
  Remediation: Escape each path segment individually, or at minimum validate that path does not contain query-string-significant characters.

• [error-handling-gap] internal/cli/lock.go — In runLock, when dep.Type == "directory", the code calls fetch.CacheGetDir and silently ignores errors (if err == nil && dirEntry != nil). If CacheGetDir returns an error (e.g., integrity check failure), the lock file entry is written without the files manifest, undermining the reproducibility guarantees that lock files are supposed to provide.
  Remediation: Propagate the error from CacheGetDir or at minimum log a warning that the file manifest could not be populated.

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being substantially modified to change the skills model from single files to directories. Per AGENTS.md: "Once an ADR on main has status Accepted, it is a point-in-time record. Do not substantially rewrite its Context, Decision, or Consequences sections." The diff shows changes to the Decision section adding the skill directory model and forge API requirements.
  Remediation: Clarify whether the original ADR was ambiguous or incorrect. If this is a correction to an architectural mistake, acknowledge it explicitly and justify inline modification vs writing a new ADR.

• [architectural-boundary-violation] internal/harness/forgeurl.go — ParseForgeURL and IsSupportedForge contain forge-specific logic (hardcoded github.com) in the harness package. Per AGENTS.md, forge-specific logic belongs in internal/forge/. When GitLab support is added, this code will need to be updated outside the forge abstraction layer.
  Remediation: Move ParseForgeURL and IsSupportedForge to internal/forge/url.go to keep forge-specific URL format knowledge in the forge abstraction layer.

Low

• [fail-open] internal/cli/lock.go — In resolveFromLock, when lockDep.Type is empty, it defaults to "file". A directory dependency incorrectly recorded without a type will be resolved as a single file, bypassing directory cache integrity checks. Low risk since runLock always sets Type from dep.Type.

• [missing-authorization] No linked issue for a non-trivial PR (20 files, 1796 additions). Filing an issue would document the rationale for this change.

• [scope-tier-mismatch] The PR uses the fix commit prefix but introduces substantial new functionality (forge API methods, directory caching, lock file schema extensions). The fix prefix will cause GoReleaser to categorize this as a bug fix in release notes. Consider feat instead.

• [test-integrity] internal/cli/lock_test.go — TestRunLock_GeneratesLockFile was modified to replace skill URL testing with policy URL testing. No test in lock_test.go verifies that runLock correctly populates Type: "directory" and Files manifest for skill directory dependencies.

• [edge-case] internal/fetch/cache.go — ComputeTreeHash with an empty files map produces a hash of "\n". CachePutDir does not reject empty file maps. Skills are required to have at least SKILL.md.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows skills/triage-coordination/SKILL.md format. Consider updating to reflect the directory model.

• [stale-skill-model-reference] docs/glossary.md:138 — Defines a skill as "A markdown file (optionally with a scripts/ directory)" which predates the current directory model.

• [incomplete-design-documentation] docs/plans/universal-harness-access-phase1.md:123 — States "supported forges (GitHub, GitLab)" but IsSupportedForge only accepts GitHub.

• [relative-url-edge-case] internal/resolve/resolve.go — If a parent URL has a trailing slash, RFC 3986 resolution would append instead of replace sibling. Low risk since ParseForgeURL doesn't produce trailing slashes.

Info

• [rate-limiting] internal/forge/github/github.go — ListDirectoryContents recursively calls itself for subdirectories when recursive=true with no depth limit or max entry count.

• [dependency-introduction] internal/resolve/resolve.go:37 — ResolveOpts gains a ForgeClient field. The code correctly checks for nil and returns a clear error message when skills require it.

[fullsend-ai-review]

See the review comment for full details.

[ralphbean]

LGTM. A few non-blocking notes inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:11 PM UTC · Completed 9:26 PM UTC
Commit: 4439417 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:39 PM UTC · Completed 9:55 PM UTC
Commit: a2a7fe2 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:01 PM UTC · Completed 10:17 PM UTC
Commit: 6602b2f · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:25 PM UTC · Completed 10:39 PM UTC
Commit: 164bd37 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:56 PM UTC · Completed 11:09 PM UTC
Commit: 63ad88f · View workflow run →

[fullsend-ai-review]

🤖 Review · Started 12:16 AM UTC
Commit: 0ca1894 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:20 AM UTC · Completed 12:33 AM UTC
Commit: 759c0de · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:54 AM UTC · Completed 1:05 AM UTC
Commit: 826f132 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:10 AM UTC · Completed 2:16 AM UTC
Commit: 826f132 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2139 — feat(resolve): model skills as directories

What happened

Human-authored PR by ggallen (24 files, +2184/−505 lines) implementing a significant architectural change to model skills as directories. The PR received 10 review agent runs over ~5.5 hours before finally being approved and merged.

Timeline:

1. PR opened at 20:35 UTC
2. First review agent run (20:36–20:52): CHANGES_REQUESTED with findings ranging Critical to Info
3. Human reviewer ralphbean approved at 20:54 with 4 non-blocking inline notes
4. Author addressed human + bot feedback, pushed fixes (~21:09)
5. Second review agent run (21:11–21:26): CHANGES_REQUESTED again (medium findings on missing lock test coverage and interface expansion)
6. Author addressed those findings, pushed fixes (~21:45)
7. Eight more review runs followed (21:39–01:05 UTC), each triggered by the author's incremental pushes
8. 10th review run (00:54–01:05): Finally APPROVED with only Low-severity findings
9. PR merged at 02:07 UTC

Assessment

The review agent provided genuinely useful feedback — the path traversal containment check, URL escaping fix, and lock test coverage additions were all surfaced by the bot and addressed by the author. However, 10 full review runs to converge is excessive, especially when a human had already approved by run #1.

The core problems observed are:

• Review churn loop: Each author push triggered a new ~15-minute review, creating a push→review→fix→push cycle
• Verdict escalation despite human approval: The bot kept issuing CHANGES_REQUESTED even after a human approved
• No incremental review capability: Each run re-reviewed the entire PR from scratch rather than focusing on what changed since the last review
• Severity instability: Initial findings rated Critical were later downgraded to Low across successive runs

Existing coverage

All major improvement areas are already tracked by open issues:

• Review deduplication/debouncing: Deduplicate review runs on rapid successive pushes #1418, Deduplicate review runs when PR is rebased multiple times in quick succession #1422, Deduplicate review dispatches for the same HEAD SHA #1452, Debounce review dispatch on rapid synchronize events #1014, Add concurrency group to dispatch-review job in fullsend.yaml #981
• Verdict calibration for human-authored PRs: Review agent: use COMMENT verdict for human-authored PRs with only medium/low findings #2115, Review agent: use COMMENT verdict for re-reviews with only low-severity findings #2029
• Human approval override prevention: Review bot should not override human approval with stale findings #1922
• Severity stabilization across re-runs: Stabilize review verdicts across re-runs on unchanged code #1389
• Review churn detection: Review agent: detect review-cycle churn and suggest approach reassessment #2111
• Incremental/prior-findings-aware reviews: Pass prior review findings to follow-up reviews for targeted verification #1552
• Concurrency groups to cancel stale runs: Add concurrency group to dispatch-review job in fullsend.yaml #981

No new proposals are warranted — the existing issue backlog comprehensively covers the patterns observed. Prioritizing implementation of #981 (concurrency groups), #2115 (COMMENT verdict for human PRs), and #1552 (incremental reviews) would have the highest impact on reducing the churn seen in this PR.