mosaicstack/stack
25
0
Fork 1
Code Issues 344 Pull Requests 13 Actions Packages Projects Releases 15 Wiki Activity

issue-close.sh: closing comment silently fails — tea issue comment is not a subcommand, and the result is unchecked #1081

New Issue
Closed
opened 2026-08-06 20:33:02 +00:00 by Ghost · 1 comment
Ghost commented 2026-08-06 20:33:02 +00:00
Copy Link
Copy Source

Summary

tools/git/issue-close.sh calls tea issue comment, which is not a subcommand of tea. On any host where a tea login is configured — the normal, correctly-configured case — the closing comment silently fails and the issue is closed anyway. The audit trail for why an issue was closed is lost, with no error surfaced.

The same file already defines a working gitea_issue_comment_api() helper, but it is only reached on the no-login fallback path.

Evidence

Measured against main @ 80a45b1e1c3b65999bd48c5b0f41f4b1cb702ff9:

$ tea --version
Version: 0.11.1

$ tea issue --help
  list · create · edit · reopen · close        <- no `comment`

$ tea --help
  comment, c   Add a comment to an issue / pr  <- it is TOP-LEVEL, not under `issue`

issue-close.sh:94 (live code, comments stripped — 1 live call site):

if [[ -n "$GITEA_LOGIN_NAME" ]]; then
    if [[ -n "$COMMENT" ]]; then
        tea issue comment "$ISSUE_NUMBER" "$COMMENT" --repo "$OWNER/$REPO" --login "$GITEA_LOGIN_NAME"
    fi
    tea issue close "$ISSUE_NUMBER" --repo "$OWNER/$REPO" --login "$GITEA_LOGIN_NAME"

Two independent defects:

  1. tea issue comment cannot succeed — the subcommand does not exist.
  2. The return value is not checked (0 || guards on it), so the script proceeds to tea issue close regardless. tea issue close is valid, so the issue closes and the comment does not post.

The no-login branch calls gitea_issue_comment_api — which works — but it is also unchecked (:100), so it carries defect 2 on its own.

Impact

This sits on the completion gate (linked issue closed). The closure outcome is unaffected; what is lost is the record of why, silently. This is the silent-failure class: an operation reports success while doing nothing.

Suggested fix

Route the comment through the existing gitea_issue_comment_api() on both branches (it is login-independent and already proven), and fail closed — do not close the issue if the comment could not be posted:

if [[ -n "$COMMENT" ]]; then
    gitea_issue_comment_api || {
        echo "Error: failed to post comment on #$ISSUE_NUMBER -- NOT closing (fail closed)." >&2
        exit 1
    }
fi

Alternatively keep tea and correct it to the top-level tea comment, but the API helper avoids depending on tea's subcommand surface and already exists in-file.

Acceptance

  • Force the failure (a comment that cannot post) and confirm from the caller's side that the issue is not closed and the exit status is non-zero.
  • A negative control: a comment that can post must still close the issue (rule-MET must pass).

Provenance note

This defect was independently fixed on a homelab host in July; that host-local edit routes both branches through the API helper with a fail-closed guard. The fix is being contributed upstream rather than left as undeclared host divergence.

No closing keywords intended; none used.

## Summary `tools/git/issue-close.sh` calls **`tea issue comment`**, which is not a subcommand of `tea`. On any host where a tea login **is** configured — the normal, correctly-configured case — the closing comment silently fails and the issue is closed anyway. The audit trail for *why* an issue was closed is lost, with no error surfaced. The same file already defines a working `gitea_issue_comment_api()` helper, but it is only reached on the **no-login fallback** path. ## Evidence Measured against `main` @ `80a45b1e1c3b65999bd48c5b0f41f4b1cb702ff9`: ``` $ tea --version Version: 0.11.1 $ tea issue --help list · create · edit · reopen · close <- no `comment` $ tea --help comment, c Add a comment to an issue / pr <- it is TOP-LEVEL, not under `issue` ``` `issue-close.sh:94` (live code, comments stripped — 1 live call site): ```bash if [[ -n "$GITEA_LOGIN_NAME" ]]; then if [[ -n "$COMMENT" ]]; then tea issue comment "$ISSUE_NUMBER" "$COMMENT" --repo "$OWNER/$REPO" --login "$GITEA_LOGIN_NAME" fi tea issue close "$ISSUE_NUMBER" --repo "$OWNER/$REPO" --login "$GITEA_LOGIN_NAME" ``` Two independent defects: 1. **`tea issue comment` cannot succeed** — the subcommand does not exist. 2. **The return value is not checked** (0 `||` guards on it), so the script proceeds to `tea issue close` regardless. `tea issue close` *is* valid, so **the issue closes and the comment does not post.** The no-login branch calls `gitea_issue_comment_api` — which works — but it is **also unchecked** (`:100`), so it carries defect 2 on its own. ## Impact This sits on the completion gate (*linked issue closed*). The closure **outcome** is unaffected; what is lost is the **record of why**, silently. This is the silent-failure class: an operation reports success while doing nothing. ## Suggested fix Route the comment through the existing `gitea_issue_comment_api()` on **both** branches (it is login-independent and already proven), and **fail closed** — do not close the issue if the comment could not be posted: ```bash if [[ -n "$COMMENT" ]]; then gitea_issue_comment_api || { echo "Error: failed to post comment on #$ISSUE_NUMBER -- NOT closing (fail closed)." >&2 exit 1 } fi ``` Alternatively keep `tea` and correct it to the top-level `tea comment`, but the API helper avoids depending on tea's subcommand surface and already exists in-file. ## Acceptance - Force the failure (a comment that cannot post) and confirm from the caller's side that the issue is **not** closed and the exit status is non-zero. - A negative control: a comment that *can* post must still close the issue (rule-MET must pass). ## Provenance note This defect was independently fixed on a homelab host in July; that host-local edit routes both branches through the API helper with a fail-closed guard. The fix is being contributed upstream rather than left as undeclared host divergence. No closing keywords intended; none used.
Ghost added the bug label 2026-08-06 20:33:02 +00:00
Ghost commented 2026-08-06 22:28:12 +00:00
Copy Link
Copy Source

⛔ Correction to this issue's severity claim — from the author

I wrote that the issue closes anyway and the record of why is silently lost. That is wrong. issue-close.sh carries set -e at line 5, so the failing tea issue comment aborts the script. Measured against main under a mocked tea:

tea login list --output json
tea issue comment 42 note --repo mosaicstack/stack --login git.mosaicstack.dev
rc=1        ⇒ `tea issue close` is NEVER reached; the issue is NOT closed

So there is no silent failure and no fail-open. set -e already fails closed, by accident rather than by design, and the "issue closes without its comment" scenario I described does not occur.

What the defect actually is

issue-close.sh -c <comment> cannot succeed at all on any host where a tea login resolves. tea issue comment is not a subcommand, so the command aborts every time. The close path is fine; the commented close is simply broken.

That is still a real bug — the documented way to close an issue with a note does not work — but it is loud, not silent, and it is a usability/availability defect rather than an audit-integrity one. I over-stated the severity and the mechanism.

What this changes about the fix

The change in #1085 is still correct and still needed, for narrower reasons:

  • tea comment (top-level) is the valid subcommand — this is the actual repair
  • the explicit || { ...; exit 1; } guard is still worth having: relying on set -e for a fail-closed property means the property silently disappears if anyone adds a || true, wraps the call in a conditional, or runs the function in a context where set -e does not apply. A safety property that holds by accident is not a control.

How I found it

The regression test I wrote for this went red on the unfixed script for the wrong reason — it fired on "wrong subcommand" rather than on "closed after the comment failed", which told me the close was never reached. Chasing that discrepancy produced the set -e finding.

The test that was supposed to prove the fix instead disproved the bug report. Recording that, because it is the argument for red-first testing better than anything I could assert.

No closing keywords intended; none used.

## ⛔ Correction to this issue's severity claim — from the author **I wrote that the issue closes anyway and the record of why is silently lost. That is wrong.** `issue-close.sh` carries `set -e` at line 5, so the failing `tea issue comment` **aborts the script**. Measured against `main` under a mocked `tea`: ``` tea login list --output json tea issue comment 42 note --repo mosaicstack/stack --login git.mosaicstack.dev rc=1 ⇒ `tea issue close` is NEVER reached; the issue is NOT closed ``` **So there is no silent failure and no fail-open.** `set -e` already fails closed, by accident rather than by design, and the "issue closes without its comment" scenario I described does not occur. ### What the defect actually is **`issue-close.sh -c <comment>` cannot succeed at all on any host where a tea login resolves.** `tea issue comment` is not a subcommand, so the command aborts every time. The close path is fine; the *commented* close is simply broken. That is still a real bug — the documented way to close an issue with a note does not work — but it is **loud, not silent**, and it is a usability/availability defect rather than an audit-integrity one. **I over-stated the severity and the mechanism.** ### What this changes about the fix The change in #1085 is still correct and still needed, for narrower reasons: - `tea comment` (top-level) is the valid subcommand — this is the actual repair - the **explicit** `|| { ...; exit 1; }` guard is still worth having: relying on `set -e` for a fail-closed property means the property silently disappears if anyone adds a `|| true`, wraps the call in a conditional, or runs the function in a context where `set -e` does not apply. **A safety property that holds by accident is not a control.** ### How I found it The regression test I wrote for this went **red on the unfixed script for the wrong reason** — it fired on "wrong subcommand" rather than on "closed after the comment failed", which told me the close was never reached. Chasing that discrepancy produced the `set -e` finding. **The test that was supposed to prove the fix instead disproved the bug report.** Recording that, because it is the argument for red-first testing better than anything I could assert. No closing keywords intended; none used.
Ghost closed this issue 2026-08-07 05:59:13 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels bug
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
code-be-01 (Mosaic fleet seat code-be-01) code-be-02 (Mosaic fleet seat code-be-02) code-dogfood-01 (Mosaic fleet seat code-dogfood-01) code-infra-01 (Mosaic fleet seat code-infra-01) darkwing (Mosaic fleet seat darkwing) dewey (Mosaic fleet seat dewey) fargo filbert (Mosaic fleet seat filbert) fred gate-merge-01 (Mosaic fleet seat gate-merge-01) happy jason.woltje (Jason Woltje) marcie merge-gate ops-01 (Mosaic fleet seat ops-01) ops-02 (Mosaic fleet seat ops-02) ops-03 (Mosaic fleet seat ops-03) ops-ci-01 (Mosaic fleet seat ops-ci-01) ops-deploy-01 (Mosaic fleet seat ops-deploy-01) orch-01 (Mosaic fleet seat orch-01) pepper resume rev-code-01 rev-code-02 rev-security-01 rev-security-02 rev-security-03 (Mosaic fleet seat rev-security-03) rocko (Mosaic fleet seat rocko) sanity scooby (Scooby) scrappy shaggy tiny topher (Mosaic fleet seat topher) velma veronica (Mosaic fleet seat veronica) vision woodpecker
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1081