fix: prevent silent partial completion — fail closed, verify pushes, persist logs #1
No reviewers
Labels
No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
riley/codetether-action!1
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/silent-partial-completion"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
Addresses issue #105: the action picked up issues, posted analysis comments, but produced no commits, no pushed branch, and no PR. This made distributed work brittle.
Root cause: The action treated all task failures as successes. Server mode hardcoded
exit_code=0regardless of task status. Push operations were never verified.Changes
Fail Closed (core fix)
poll_task_result— now returns non-zero on failure/timeout/canceled (was always returning 0)handle_issue— checks dispatch+poll results, exits non-zero on failure (was ignoring errors)run_server_review— exits non-zero when dispatch or poll fails (was hardcoding exit_code=0)run_local_review— exits non-zero when codetether binary fails (was swallowing the exit code)apply_fix— usesverified_git_pushthat confirms the commit landed on remotePush Verification
verified_git_push— pushes, then queries GitHub API to confirm the commit SHA is at the branch tipverify_branch_pushed— checks a branch exists on remote via APIverify_commit_on_branch— confirms expected SHA matches remote branch HEADStatus Comments (user visibility)
$GITHUB_STEP_SUMMARYat every exit pathLog & Artifact Persistence
.codetether-logs/action.log(survives action failure)actions/upload-artifact@v4step runsif: always()with 14-day retentionNew Inputs/Outputs
fail_on_errorinput (default:true) — setfalsefor best-effort modetask_idoutput — exposes server task ID for downstream useGITHUB_STEP_SUMMARY,GITHUB_RUN_ID,GITHUB_SERVER_URLenv vars now passed throughInfrastructure
log_info/log_warn/log_error— structured logging to both console and persistent filefinalize_run— writes markdown summary, called at every exit pathcreate_pull_request— PR creation helper with retry and duplicate detectiongithub_api_post— POST helper with status check and error logging\nliterals in comment bodies → proper shell newlines.gitignorenow excludes.codetether-agent/Files Changed
action.ymlaction/entrypoint.shaction/lib/common.shaction/lib/github.shaction/lib/server.shaction/lib/local.shAPI-Side Companion Changes Needed
The server-side task dispatch API (
/v1/tasks/dispatch) and its workers should also be hardened:/v1/tasks/dispatch/{id}should include anerror_messagefield (not juststatus=failed) so the action can surface the actual failure reasonfiles_changedlist andgit_operationsarray so the action can verify the worker actually made changesTesting
bash -nsyntax validationfix/silent-partial-completionCode Review
This pull request introduces comprehensive logging, artifact persistence, and "fail closed" logic to the CodeTether GitHub Action. Key changes include the addition of a fail_on_error input, structured logging to both the console and a persistent log file, and automated status comments on GitHub issues and PRs. The action now supports uploading logs as artifacts and generating a detailed job summary. Feedback focuses on improving the robustness of shell script variable assignments when capturing HTTP status codes from curl, specifically to prevent non-numeric values from causing syntax errors or unexpected crashes during integer comparisons.
@ -1,18 +1,25 @@#!/usr/bin/env bashThe use of
2>&1inside the subshell captures both stdout (the HTTP status code) and stderr (potential curl error messages) into therespvariable. If a network error occurs,respwill contain a string likecurl: (6) Could not resolve host..., which will cause a syntax error in the subsequent integer comparison at line 17 ([ "$resp" -ge 200 ]). It is safer to redirect stderr to the log file and provide a default value if curl fails.Similar to
post_github_comment, capturing stderr intohttp_codecan lead to non-numeric values and crashes in comparisons. Additionally, sinceset -eis active, a curl failure here will crash the script immediately without a proper error message or summary. Redirecting stderr to the log file and ensuring a default numeric value is more robust.@ -56,22 +57,32 @@ dispatch_server_task() {fiCapturing stderr via
2>&1and piping toteeresults inhttp_codecontaining both the status code and any error messages. This will cause the integer comparison at line 67 to fail. Redirecting stderr directly to the log file ensures the variable only contains the status code, and using$?directly is safer thanPIPESTATUSwhen the pipe is removed.Pull request overview
This PR hardens the CodeTether GitHub Action against “silent partial completion” by failing closed on task failures, persisting diagnostics as artifacts, and verifying that git pushes actually landed on the remote.
Changes:
.codetether-logs/and upload them viaactions/upload-artifact@v4 (if: always()).Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
action/lib/server.shaction/lib/local.shaction/lib/github.shaction/lib/common.shfinalize_runstep summary writer.action/entrypoint.shaction.ymlfail_on_error,task_idoutput, passes env vars, and uploads logs artifact..gitignore.codetether-agent/.💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -146,7 +156,10 @@ runs:INPUT_AGENT_TYPE: ${{ inputs.agent_type }}action.ymlsetsGITHUB_STEP_SUMMARY: ${{ github.step_summary }}.GITHUB_STEP_SUMMARYis already provided by the runner as an environment variable, andgithub.step_summaryis not a standard context value; this risks overwriting the real path with an empty string and preventing summaries from being written. Drop this env mapping and rely on the runner-providedGITHUB_STEP_SUMMARYinstead.@ -68,16 +71,59 @@ ${comment_instructions}Issue body:In
handle_issuelocal-mode path,if ! run_local_codetether ...; then local ec=$?captures the status of the!expression (usually 0) rather than codetether’s real exit code. Capture the exit code fromrun_local_codetetherdirectly before negation/branching so logs andwrite_review_outputreflect the true failure.finalize_runappends the entire persistent log file into$GITHUB_STEP_SUMMARY. Action logs can get very large (especially withtee), which can exceed GitHub’s step summary size limits and make the summary noisy/unusable. Consider truncating (e.g., last N KB/lines) or linking to the uploaded artifact instead of embedding the full file.@ -16,3 +22,4 @@fi}# ── Write multi-line review output to $GITHUB_OUTPUT ─────────────post_github_commentcaptures curl output with-w "%{http_code}"but also redirects2>&1into the same variable and then does numeric comparisons. If curl writes an error message,respwon’t be a pure number and the HTTP check becomes unreliable. Keep stderr separate (or check curl’s exit code) and ensure the variable contains only the status code.@ -47,0 +88,4 @@elselog_error "Branch '${branch}' NOT found on remote (HTTP ${ref_response})"return 1figithub_api_postassignshttp_codefromcurl ... -w "%{http_code}"while also redirecting2>&1into the same capture; any stderr output will corrupt the code and can break the HTTP range check. Also consider explicitly handling curl non-zero exit (underset -e, a failing curl inside$(...)may exit the script before this function can return 1).@ -47,0 +160,4 @@sleep 2donelog_error "Failed to create PR after ${max_retries} attempts"verify_commit_on_branchdoesjqparsing on$responsewithout guarding for empty/non-JSON responses. Since the action runs withset -e, a transient GitHub API failure could causejqto exit non-zero and abort the entire action instead of returning 1. Handle API failures by validating the response (or usingjq -ewith|| true) and returning a controlled non-zero status.@ -164,39 +179,63 @@ After editing files, run the smallest relevant validation needed to support thelocal fix_filefix_file="$(mktemp)"In
apply_fix, the patternif ! run_local_codetether ...; then local ec=$?records the exit code of the negated condition (typically 0), not the actual codetether exit code. Run the command without!, capture$?immediately, and branch on the captured value so failures report the real exit status.@ -56,22 +57,32 @@ dispatch_server_task() {firesponse="$(cat /tmp/codetether-response.json)"will cause the whole action to exit underset -eif curl failed before writing the file. Read the file defensively (e.g.,cat ... 2>/dev/null || true) and treat missing/empty response as a dispatch failure with a usefulREVIEW_TEXT.dispatch_server_taskassignshttp_codefromcurl ... 2>&1 | tee ..., which will mix curl stderr/log output into the variable and can make the subsequent numeric HTTP checks unreliable. Capture the HTTP code cleanly (stdout only) and tee stderr separately; also handlecurlnon-zero exits explicitly (you already computecurl_exitbut never use it).Addressed Review Feedback
Commit:
6590181All 11 review comments addressed across 2 reviewers (gemini-code-assist, Copilot). Summary of fixes:
curl stderr isolation (6 comments)
All curl calls that used
2>&1to capture stderr into the same variable as the HTTP status code now redirect stderr to the log file (2>> "${CODETETHER_LOG_FILE}") and default to"000"on empty response. Affected functions:post_github_comment—2>&1→2>> logfile+|| :+ fallbackgithub_api_post— same patternverify_branch_pushed— same patterndispatch_server_task— removed2>&1 | teepipe, direct capture with0Defensive response handling (2 comments)
dispatch_server_task: reads/tmp/codetether-response.jsonwith2>/dev/null || true, treats empty response as dispatch failureverify_commit_on_branch:jqcall wrapped with2>/dev/null || trueto preventset -eabort on bad JSONExit code capture fix (2 comments)
handle_issue(entrypoint.sh): changedif ! run_local_codetether ...; then local ec=$?torun_local_codetether; local ec=$?; if [ "$ec" -ne 0 ]— captures the real exit code instead of the negated resultapply_fix(local.sh): same fix appliedStep summary log truncation (1 comment)
finalize_run: when log exceeds 10 KB, embeds only the last 10 KB with a note about the full artifact instead of dumping the entire fileRemove invalid env mapping (1 comment)
action.yml: removedGITHUB_STEP_SUMMARY: ${{ github.step_summary }}— the runner provides this natively;github.step_summaryis not a valid context and was overwriting with emptyTests
bash -nsyntax validation