rev-974 (#1085 review 130) found three blockers. This closes the first two.
[1] SANDBOX ESCAPE. The test ran under `set -uo pipefail` with unchecked mkdir,
calls-log redirect and `cd "$REPO_DIR"`, then prepended a possibly-nonexistent
$MOCK_BIN to PATH -- while `git remote add origin` names the REAL repository.
rev-974 forced setup failure with an unwritable AGENT_WORK_ROOT and the test
continued past every error, ran `git init` in its CALLER's directory, added the
real origin, and invoked its target:
TARGET_REACHED args=-i 42 -c closing note
With the committed target that is the real, provider-mutating issue-close.sh.
ShellCheck flagged the unguarded cd as SC2164 independently.
Now: `set -euo pipefail`, every setup step checked with a legible reason, and
assert_mocked() proves BOTH `tea` and `curl` resolve inside $MOCK_BIN before any
target invocation. Control: unwritable AGENT_WORK_ROOT -> rc=1 at mkdir, target
never reached.
[2] THE API/no-login BRANCH HAD NO DISCRIMINATING COVERAGE. The mock always returned
a tea login, so `gitea_issue_comment_api || fail-closed` was never executed.
rev-974 replaced the whole fallback contract with an unconditional close --
silently dropping the comment -- and the committed test still passed rc=0.
Added three cases asserting the POSTCONDITION (which HTTP calls happened, in what
order) rather than that a command ran: comment POST fails -> no PATCH and non-zero;
comment succeeds -> strictly POST,PATCH; no comment requested -> PATCH only, never
a POST. The curl mock now records method and URL. Control: replaying rev-974's
contract destruction now fails with "API path: no comment POST attempted".
Two self-inflicted traps hit while adding `set -e`, both the same family as the #1086
defect be-coder-08 found, and both silent:
- `grep -q X "$CALLS" && fail "..."` -- the ABSENT case (grep rc=1, the PASSING case
for a must-not-appear assertion) is the last command of an && list and terminates
the script with no message. All four converted to if-blocks.
- `run_target ...; rc=$?` -- the function's non-zero RETURN trips set -e in the CALLER
before rc is read; run_target's internal `set +e` protects the target, not the
caller. All five call sites now `rc=0; run_target ... || rc=$?`.
Controls:
unchanged main -> rc=1 "used 'tea issue comment'"
fallback contract destroyed -> rc=1 "API path: no comment POST attempted"
unwritable AGENT_WORK_ROOT -> rc=1 at setup, target never invoked
fixed source -> rc=0
[3] remains open: with MOSAIC_GIT_IDENTITY=rev-974 the wrapper resolves
GITEA_LOGIN_NAME=mosaicstack-mos, so one principal holds but the operation is
attributed to Mos rather than the requested seat. That changes identity resolution
shared by every wrapper in this directory and is not folded in here.
Reported-by: rev-974
Addresses both blockers from review 127 (rev-security-02) and corrects the
severity claim in the original report.
Blocker 1 -- mixed principals. Routing the comment through the token-
authenticated gitea_issue_comment_api() attributed the comment to the token
holder while the close still used --login $GITEA_LOGIN_NAME: two principals for
one operation. `tea comment` accepts the same --repo/--login flags, so the tea
branch now uses it and both calls carry the same principal. The no-login branch
keeps the API helper for both, also a single principal.
Blocker 2 -- no regression test. Adds test-issue-close-fail-closed.sh on the
existing mocked-tea/sandboxed-git harness pattern. Asserts: a failed comment
does not close the issue and exits non-zero; a successful comment does close it;
the subcommand is top-level `tea comment`, never `tea issue comment`; and the
comment and close carry the same --login. GREEN on this branch, RED on main.
Severity correction. The original report said the issue closes anyway and the
audit trail is silently lost. It does not: set -e at line 5 aborts the script
when the comment fails, so the close is never reached. The real defect is that
issue-close.sh -c cannot succeed at all where a tea login resolves -- loud, not
silent. The explicit || guard is retained deliberately: a fail-closed property
that depends on set -e disappears the moment anyone adds `|| true` or wraps the
call in a conditional. Posted as a comment on #1081 and #1085.
Refs #1081
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Amf1Neca162odgcbCWMk1y
`tea issue comment` is not a subcommand -- tea 0.11.x exposes only
list/create/edit/reopen/close under `tea issue`, and comments are the
top-level `tea comment`. The call therefore always failed. Its result was
never checked, so the script went on to `tea issue close`, which IS valid:
the issue closed and the record of why it closed was silently lost.
Route the comment through the existing gitea_issue_comment_api() helper on
both branches. It is login-independent and was already the mechanism used by
the no-login fallback. Both call sites now fail closed: if the comment cannot
be posted, the issue is not closed.
Verified behaviourally against the live provider, both directions:
negative -- comment cannot post => "NOT closing (fail closed)", exit 1,
issue left open
positive -- comment posts and issue closes => state=closed, comments=1, exit 0
Refs #1081
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Amf1Neca162odgcbCWMk1y
Mos
Mos
be-coder-08andMos
coder-mos1andmos-dt-0
jason.woltjeandMos