fix(release): use relative paths in find command for dist/ staging#168
Conversation
The release workflow was recently refactored to consolidate the build output from .github/actions/*/dist/ into dist/ at the repo root. The old find command used relative paths from the workspace root: find .github/actions/setup-credentials/dist/ .github/actions/signed-commit/dist/ -type f The new command uses the full GITHUB_WORKSPACE prefix, producing absolute paths: find "$GITHUB_WORKSPACE/dist/" -type f -> /home/runner/work/cagent-action/cagent-action/dist/credentials.js signed-commit.js forwards these paths verbatim as git file paths in the GitHub createCommitOnBranch GraphQL mutation. The GitHub API requires paths relative to the repository root; it rejects absolute paths with the cryptic error: "A path was requested for deletion which does not exist as of commit oid `8088333d...`" The PATCH /git/refs/... - 422 line in the Actions log is from updateRef (branch did not exist yet, correctly caught and handled via createRef); the actual failure is the subsequent GraphQL mutation receiving invalid absolute paths. Fix: - Use `find dist/ -type f` (relative) in the Pass 1 staging command. - Add assertRelativePath() validation in signed-commit/index.ts so absolute paths fail fast with a clear message instead of a cryptic GitHub API error. - Add a delete-then-recreate fallback in signed-commit.ts for the rare case where a stale staging branch exists with incompatible state that prevents force-update (422 that is not 'Reference does not exist'). - Add unit tests for the new fallback paths. Assisted-By: docker-agent
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The changes are well-scoped and correctly address the root cause. No bugs found in the added code.
What was reviewed:
.github/workflows/release.yml—find dist/ -type f(relative path fix) ✅src/signed-commit/index.ts—assertRelativePath()guard ✅src/signed-commit/signed-commit.ts— delete-then-recreate fallback for stale 422 ✅src/signed-commit/__tests__/signed-commit.test.ts— new unit tests ✅
Notes:
- The
assertRelativePath()function correctly usesisAbsolute()fromnode:pathand provides an actionable error message with a concrete fix hint. All three call sites (--add,--delete,--add-stdin,--delete-stdin) are covered. - The
else if (status === 422)branch correctly gates on a non-"Reference does not exist" 422, avoiding conflict with the existing branch-creation path. The swalloweddeleteReferror is intentional and safe (concurrent deletion race). - New tests cover both the happy path (delete succeeds, recreate proceeds) and the defensive path (delete fails, recreate still proceeds). Both correctly assert
mockGraphqlis called, confirming the commit completes. - The workflow comment added above the
findcommand is accurate and will help future maintainers avoid reintroducing the bug.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR correctly fixes the root cause (absolute paths passed to GitHub's GraphQL API via ) and adds a meaningful defense-in-depth guard () and a stale-branch recovery fallback in . The test coverage for the new paths is solid.
One medium-severity issue and two minor notes were identified in the new code.
| } catch { | ||
| // Ignore — the branch may have been concurrently deleted. | ||
| } | ||
| await octokit.rest.git.createRef({ |
There was a problem hiding this comment.
[NOTABLE] createRef called unconditionally after silenced deleteRef failure
When deleteRef throws and the exception is swallowed by the empty catch block, createRef is called unconditionally. The comment assumes the only reason deleteRef would fail is a concurrent deletion, but it can also fail due to permissions issues, transient network errors, or API quota limits. In those cases the branch still exists and the subsequent createRef will throw a 422 "Reference already exists" error with no helpful context, masking the original failure.
Trigger path: force=true → updateRef throws 422 (non-"does not exist") → deleteRef throws for non-concurrent-deletion reason (e.g. permissions) → catch {} swallows it → createRef fails with 422.
Suggestion: Consider re-throwing (or at least logging) deleteRef failures that aren't 404/422, or wrapping the subsequent createRef to produce a more informative error:
} catch (deleteErr) {
// Only ignore if branch was already gone
const deleteStatus = (deleteErr as { status?: number }).status;
if (deleteStatus !== 404 && deleteStatus !== 422) throw deleteErr;
}| # signed-commit uses the paths verbatim as git file paths in the GitHub API, which | ||
| # requires repo-root-relative paths. Absolute paths cause a cryptic | ||
| # "A path was requested for deletion which does not exist" GraphQL error. | ||
| TEMP_SHA=$(find dist/ -type f 2>/dev/null | \ |
There was a problem hiding this comment.
[MINOR] find dist/ relies on implicit CWD being the repository root
The fix correctly switches from find "$GITHUB_WORKSPACE/dist/" -type f (absolute path → breaks GraphQL API) to find dist/ -type f (relative path). GitHub Actions defaults to $GITHUB_WORKSPACE as the working directory for run: steps, so this works in the current setup.
However, there's no explicit working-directory: guard. If this step or its job ever gains a working-directory: setting, or if the actions/checkout step is given a path: option, find dist/ would silently enumerate zero files. Since --add-stdin only reads whatever was piped in, an empty input would produce an empty/garbled TEMP_SHA, causing a confusing downstream failure.
Suggestion: Either add an explicit check after the find or note the implicit dependency in a comment (a comment already explains the relative-path requirement, so this is low risk):
TEMP_SHA=$(find dist/ -type f 2>/dev/null | ...)
if [ -z "$TEMP_SHA" ]; then
echo "::error::TEMP_SHA is empty — ensure working directory is repo root and dist/ exists"
exit 1
fi| ref: `refs/heads/${branch}`, | ||
| sha: headSha, | ||
| }); | ||
| } else if (status === 422) { |
There was a problem hiding this comment.
[MINOR] Broad else if (status === 422) catch may silently mask unrelated API errors
The new else if (status === 422) branch catches all 422 responses from updateRef that don't contain "Reference does not exist". This covers the stale-branch scenario correctly, but would also catch unrelated 422s — for example, an "Invalid ref name" GitHub error — triggering a destructive delete+recreate instead of surfacing the error to the caller.
In practice the risk is low since invalid ref names would be caught before updateRef is called, but it's worth documenting the intent explicitly.
Suggestion: Narrow the catch by checking for known stale-branch error substrings (similar to how the first branch checks errMessage.includes('Reference does not exist')), or add a comment clarifying that all remaining 422s from updateRef are intentionally treated as stale-branch conditions.
Three changes from code review of PR #168: 1. [NOTABLE] Capture deleteRef error; only swallow 404 (race-condition concurrent deletion). All other deleteRef failures are now wrapped with context linking back to the original updateRef error and re-thrown so createRef is never called blindly. 2. [MINOR] Add `working-directory: ${{ github.workspace }}` to the 'Create release commit with pinned refs' step, making the CWD assumption explicit rather than relying on the GitHub Actions default. 3. [MINOR] Narrow the `else if (status === 422)` stale-branch path to only fire when GitHub returns 'Reference already exists'. Any other 422 (invalid ref name, permissions, etc.) is now re-thrown unchanged instead of triggering a silent delete+recreate. Tests updated accordingly: replace the two original stale-branch tests with four targeted cases covering the narrowed trigger condition, the re-throw path for unknown 422s, the 404 race-condition pass-through, and the non-race deleteRef failure wrap-and-rethrow. All 34 unit tests pass. Assisted-By: docker-agent
Root Cause
The release workflow was recently refactored to consolidate build output from
.github/actions/*/dist/intodist/at the repo root (commit83bd0cd— swap Vite for tsup). The oldfindcommand used relative paths from the workspace root:The new command uses
$GITHUB_WORKSPACEas the prefix, producing absolute paths:signed-commit.jsforwards these paths verbatim as git file paths in the GitHubcreateCommitOnBranchGraphQL mutation. The GitHub API requires paths relative to the repository root; it rejects absolute paths with the cryptic error seen in run #25339253976:Changes
.github/workflows/release.ymlChanged Pass 1's
findto use a relative path:Added an inline comment explaining why absolute paths must not be used here.
src/signed-commit/index.tsAdded
assertRelativePath()validation so callers get a clear error if an absolute path is passed, instead of a cryptic GitHub API error:src/signed-commit/signed-commit.tsAdded a delete-then-recreate fallback for the edge case where a stale staging branch exists with incompatible state preventing force-update (any 422 that is not "Reference does not exist"). Prevents the workflow from getting stuck if the cleanup trap fails on a prior run.
src/signed-commit/__tests__/signed-commit.test.tsAdded two new unit tests covering the new fallback paths.
Testing
All 32 unit tests pass (
pnpm test). TypeScript type-check passes (pnpm typecheck). Build succeeds (pnpm build).Refs run #25339253976