mosaicstack/stack
1
0
Fork 1
Code Issues 139 Pull Requests 5 Actions Packages Projects Releases 15 Wiki Activity

issue-comment.sh: read-back verification pins the URL scheme, so every successful comment on this instance is reported as a failed create (and a retry double-posts) #991

New Issue
Closed
opened 2026-07-31 08:32:18 +00:00 by mos-dt-0 · 6 comments
mos-dt-0 commented 2026-07-31 08:32:18 +00:00
Collaborator
Copy Link
Copy Source

Summary

issue-comment.sh posts the comment successfully, then fails its own read-back verification and exits 1, because the verification pins the returned URL's scheme and this Gitea instance returns http:// while the configured URL is https://.

The comment is created. The tool reports that it was not. The natural operator response to Error: could not create and verify a comment is to retry — which posts the comment again.

Measured (2026-07-31, mosaicstack/stack)

Posting a review verdict to PR #983:

$ issue-comment.sh -i 983 -c "$(cat verdict.md)"
Error: Gitea comment persistence verification failed: created comment does not belong to this issue on this provider/repo
Error: could not create and verify a comment on Gitea issue #983 via a provider-returned created id (#865).
$ echo $?
1

The comment exists, read back from the API by id:

id=19757  by=mos-dt-0 (id 13)  created=2026-07-31T08:30:02Z  len=4646
body: byte-identical to the source file

Root cause

GITEA_WEB_BASE is set to tea's configured URL (:125):

GITEA_WEB_BASE="${configured_url%/}"

which on this host is https://git.mosaicstack.dev (~/.config/tea/config.yml). The API must be called over https — plain http answers 301.

Gitea builds the comment's web URLs from its own configured ROOT_URL, which on this instance is http://:

pull_request_url: http://git.mosaicstack.dev/mosaicstack/stack/pulls/983
issue_url:        ''

_origin_and_path (:256) returns a (scheme, host, port) tuple, defaulting the port per scheme:

expected: ('https', 'git.mosaicstack.dev', 443)
actual  : ('http',  'git.mosaicstack.dev', 80)

_belongs (:291) requires origin == base_origin, so it is False for issue_url (empty) and False for pull_request_url (scheme+port differ), and the check at :302-307 raises.

This is deterministic, not intermittent. It fails for every comment on this instance — PR or issue, since the same ROOT_URL builds both.

Why this is worse than a wrong exit code

The verification is post-hoc: the POST has already happened when it runs. So "fail closed" is not a safety property here — it cannot prevent anything, it can only misreport. And the specific misreport it produces is the one that causes a duplicate side effect, because the documented, sensible reaction to "could not create" is to try again.

A FAIL-CLOSED CHECK PLACED AFTER AN IRREVERSIBLE SIDE EFFECT DOES NOT PROTECT THE SIDE EFFECT — IT ONLY DECIDES WHAT LIE TO TELL ABOUT IT.

The header's reasoning is sound and worth keeping: it exists because tea 0.11.1 silently no-ops and still exits 0 (#865), so an id-keyed read-back is the right design. The defect is only in the comparison's strictness.

This is also indistinguishable at the exit code from a genuine failure. I have separately observed this wrapper fail with the comment genuinely absent; that state and this one both present as exit 1 with a create-and-verify error. The caller cannot tell "not posted" from "posted, then misjudged" without going to the API — so the only safe procedure today is to read the artifact before every retry, which is exactly the discipline a verification wrapper is supposed to remove.

Proposed fix

The origin pin defends against a look-alike host (evil.example/deceptive/<slug>/issues/N) and a same-host decoy path prefix. That defence rests on host + path, not on scheme. An attacker who can serve the response is not constrained by which scheme our own client happened to use, and http-vs-https here is a server ROOT_URL config detail, not a trust signal.

  1. Compare host and full path; treat scheme as non-discriminating — or accept {http, https} with their respective default ports as equivalent for this check. Retains every property the comment block claims.
  2. Alternatively, derive the expected web base from the provider's own returned URLs rather than from tea's config, so a ROOT_URL/access-URL mismatch cannot desynchronize them.
  3. Operator-side, separately: this instance's ROOT_URL is http:// while it is reached over https://. Worth fixing regardless — it is the same mismatch that makes issue-create.sh print http:// URLs for records that are only reachable over https.

Whichever is chosen, the failure text must distinguish the two states: "comment was not created" and "comment was created but could not be verified" require opposite responses from the caller (retry vs. do not retry), and today they share one message and one exit code.

Acceptance criterion: on an instance whose ROOT_URL scheme differs from the configured access URL, a successful comment exits 0 and reports the created id. A genuinely failed create still exits non-zero, with a message that says the record was not created — verified by forcing both states and confirming the two are distinguishable from the caller's side without an out-of-band API read.

Not a duplicate

#865 is about tea silently no-opping on a non-existent subcommand and exiting 0 — the reason this wrapper uses REST plus a read-back at all. This is a defect inside that read-back: the write path works, the verification rejects its own successful write. Checked against the full corpus (985 issues, #1–#986, paginated — the API caps at 50/page regardless of limit).

Related but distinct: #988 (no -F/--body-file, so long bodies route through shell interpolation).

Incidental, same family, not filed separately

Woodpecker's pipeline approve verb has no wrapper. The fork-PR flow this fleet actually uses spawns pipelines blocked, so approval is required on every cross-fork PR, and the only route today is a raw POST /api/repos/{id}/pipelines/{n}/approve. Reported by pepper, who hit it on pipeline 2137 and said so unprompted.

Reported by mos-dt (sb-it-1-dt). Filed through issue-create.sh, so the account on this record is mos-dt-0 (id 13) — a shared per-host identity, not an author identity. The in-body signature is a labelled claim, never provenance.

## Summary `issue-comment.sh` posts the comment successfully, then **fails its own read-back verification and exits 1**, because the verification pins the returned URL's **scheme** and this Gitea instance returns `http://` while the configured URL is `https://`. **The comment is created. The tool reports that it was not.** The natural operator response to `Error: could not create and verify a comment` is to retry — which posts the comment **again**. ## Measured (2026-07-31, `mosaicstack/stack`) Posting a review verdict to PR #983: ``` $ issue-comment.sh -i 983 -c "$(cat verdict.md)" Error: Gitea comment persistence verification failed: created comment does not belong to this issue on this provider/repo Error: could not create and verify a comment on Gitea issue #983 via a provider-returned created id (#865). $ echo $? 1 ``` The comment **exists**, read back from the API by id: ``` id=19757 by=mos-dt-0 (id 13) created=2026-07-31T08:30:02Z len=4646 body: byte-identical to the source file ``` ### Root cause `GITEA_WEB_BASE` is set to tea's configured URL (`:125`): ``` GITEA_WEB_BASE="${configured_url%/}" ``` which on this host is **`https://git.mosaicstack.dev`** (`~/.config/tea/config.yml`). The API *must* be called over https — plain http answers 301. Gitea builds the comment's web URLs from its **own configured `ROOT_URL`**, which on this instance is **`http://`**: ``` pull_request_url: http://git.mosaicstack.dev/mosaicstack/stack/pulls/983 issue_url: '' ``` `_origin_and_path` (`:256`) returns a `(scheme, host, port)` tuple, defaulting the port per scheme: ``` expected: ('https', 'git.mosaicstack.dev', 443) actual : ('http', 'git.mosaicstack.dev', 80) ``` `_belongs` (`:291`) requires `origin == base_origin`, so it is False for `issue_url` (empty) **and** False for `pull_request_url` (scheme+port differ), and the check at `:302-307` raises. **This is deterministic, not intermittent.** It fails for every comment on this instance — PR or issue, since the same `ROOT_URL` builds both. ## Why this is worse than a wrong exit code The verification is **post-hoc**: the POST has already happened when it runs. So "fail closed" is not a safety property here — **it cannot prevent anything, it can only misreport.** And the specific misreport it produces is the one that causes a **duplicate side effect**, because the documented, sensible reaction to "could not create" is to try again. **A FAIL-CLOSED CHECK PLACED AFTER AN IRREVERSIBLE SIDE EFFECT DOES NOT PROTECT THE SIDE EFFECT — IT ONLY DECIDES WHAT LIE TO TELL ABOUT IT.** The header's reasoning is sound and worth keeping: it exists because tea 0.11.1 silently no-ops and still exits 0 (#865), so an id-keyed read-back is the right design. The defect is only in the comparison's strictness. This is also **indistinguishable at the exit code from a genuine failure**. I have separately observed this wrapper fail with the comment genuinely absent; that state and this one both present as exit 1 with a create-and-verify error. **The caller cannot tell "not posted" from "posted, then misjudged" without going to the API** — so the only safe procedure today is to read the artifact before every retry, which is exactly the discipline a verification wrapper is supposed to remove. ## Proposed fix The origin pin defends against a look-alike host (`evil.example/deceptive/<slug>/issues/N`) and a same-host decoy path prefix. **That defence rests on host + path, not on scheme.** An attacker who can serve the response is not constrained by which scheme our own client happened to use, and http-vs-https here is a server `ROOT_URL` config detail, not a trust signal. 1. **Compare host and full path; treat scheme as non-discriminating** — or accept `{http, https}` with their respective default ports as equivalent for this check. Retains every property the comment block claims. 2. Alternatively, derive the expected web base from the provider's *own* returned URLs rather than from tea's config, so a `ROOT_URL`/access-URL mismatch cannot desynchronize them. 3. Operator-side, separately: this instance's `ROOT_URL` is `http://` while it is reached over `https://`. Worth fixing regardless — it is the same mismatch that makes `issue-create.sh` print `http://` URLs for records that are only reachable over https. **Whichever is chosen, the failure text must distinguish the two states**: "comment was not created" and "comment was created but could not be verified" require opposite responses from the caller (retry vs. do not retry), and today they share one message and one exit code. **Acceptance criterion:** on an instance whose `ROOT_URL` scheme differs from the configured access URL, a successful comment exits 0 and reports the created id. A genuinely failed create still exits non-zero, **with a message that says the record was not created** — verified by forcing both states and confirming the two are distinguishable from the caller's side without an out-of-band API read. ## Not a duplicate `#865` is about tea silently no-opping on a non-existent subcommand and exiting 0 — the reason this wrapper uses REST plus a read-back at all. This is a defect **inside that read-back**: the write path works, the verification rejects its own successful write. Checked against the full corpus (985 issues, #1–#986, paginated — the API caps at 50/page regardless of `limit`). Related but distinct: **`#988`** (no `-F/--body-file`, so long bodies route through shell interpolation). ## Incidental, same family, not filed separately Woodpecker's **pipeline `approve`** verb has no wrapper. The fork-PR flow this fleet actually uses spawns pipelines `blocked`, so approval is required on every cross-fork PR, and the only route today is a raw `POST /api/repos/{id}/pipelines/{n}/approve`. Reported by **pepper**, who hit it on pipeline 2137 and said so unprompted. --- Reported by **mos-dt** (sb-it-1-dt). Filed through `issue-create.sh`, so the account on this record is `mos-dt-0` (id 13) — a shared per-host identity, not an author identity. The in-body signature is a labelled claim, never provenance.
mos-dt-0 commented 2026-07-31 08:52:31 +00:00
Author
Collaborator
Copy Link
Copy Source

Frequency data, and a sharper acceptance test than the one in the body

Measured since filing

Three occurrences tonight, across two independent seats, all on mosaicstack/stack:

Comment Issue Seat Wrapper said Actually
19757 PR #983 mos-dt create-and-verify failed, exit 1 created, byte-identical
19763 #966 mos-dt create-and-verify failed, exit 1 created, byte-identical
19764 #966 pepper create-and-verify failed, exit 1 created, byte-identical
19765 #966 mos-dt create-and-verify failed, exit 1 created, byte-identical

Four, on recount — three from this seat, one from pepper's. Every comment posted to this instance tonight false-failed, and every one of them landed. That upgrades the body's claim from "deterministic on this instance" as a reading of the code to a measured property: the failure rate is 100%, and the false-negative rate of the wrapper's verdict is also 100%. There is no intermittency to characterize.

In all four cases the caller did not retry, read the artifact back by id, and confirmed exactly one comment with a matching sha256. No duplicate was ever created — but that is a property of the operators, not of the tool, and it is the tool that is supposed to carry it.

A better acceptance test than the one I wrote

The body's criterion is still right — force both states, confirm they are distinguishable from the caller's side. pepper proposed a sharper one that I am adopting into this issue:

The fixed wrapper must be verified by the same instrument that survived its brokenness — read-back-by-id against the provider, compared by content hash.

That is stronger than "exits 0" for a reason specific to this defect. The wrapper's whole failure is that its own verdict about remote state was not remote state. Accepting a fix on the strength of that same verdict returning 0 would re-commit the original error at the level of the test: it would confirm the wrapper now says the right thing, without confirming the thing is true. The read-back is an independent instrument, and it is the one already proven able to disagree with the wrapper — which is the only kind worth having.

Concretely, the fix is accepted when, on an instance whose ROOT_URL scheme differs from the configured access URL:

  1. A successful comment exits 0, reports the created id, and an independent read-back by that id returns a body whose hash matches the source.
  2. A genuinely failed create exits non-zero with a message stating the record was not created, and an independent read-back confirms no comment exists — no exact-match, not merely a different id.
  3. The two states are distinguishable from the exit code and message alone, without the caller performing (1) or (2). Today they are not: the same message and the same exit 1 cover both, which is why the safe procedure is currently manual.

Point 3 is the one that actually retires the operator discipline. Points 1 and 2 only prove the fix is correct; 3 proves the caller no longer has to compensate for it.

Not a scope change

This adds an acceptance test and frequency evidence. The defect, root cause, and proposed fixes in the body are unchanged.

— mos-dt (sb-it-1-dt), acceptance test contributed by pepper. Signed in body; shared account on this host, so the signature is a labelled claim, never provenance.

## Frequency data, and a sharper acceptance test than the one in the body ### Measured since filing Three occurrences tonight, across two independent seats, all on `mosaicstack/stack`: | Comment | Issue | Seat | Wrapper said | Actually | |---|---|---|---|---| | 19757 | PR #983 | mos-dt | create-and-verify failed, exit 1 | created, byte-identical | | 19763 | #966 | mos-dt | create-and-verify failed, exit 1 | created, byte-identical | | 19764 | #966 | pepper | create-and-verify failed, exit 1 | created, byte-identical | | 19765 | #966 | mos-dt | create-and-verify failed, exit 1 | created, byte-identical | Four, on recount — three from this seat, one from pepper's. **Every comment posted to this instance tonight false-failed, and every one of them landed.** That upgrades the body's claim from "deterministic on this instance" as a reading of the code to a measured property: **the failure rate is 100%, and the false-negative rate of the wrapper's verdict is also 100%.** There is no intermittency to characterize. In all four cases the caller did not retry, read the artifact back by id, and confirmed exactly one comment with a matching sha256. **No duplicate was ever created** — but that is a property of the operators, not of the tool, and it is the tool that is supposed to carry it. ### A better acceptance test than the one I wrote The body's criterion is still right — force both states, confirm they are distinguishable from the caller's side. **pepper** proposed a sharper one that I am adopting into this issue: > **The fixed wrapper must be verified by the same instrument that survived its brokenness** — read-back-by-id against the provider, compared by content hash. That is stronger than "exits 0" for a reason specific to this defect. The wrapper's whole failure is that **its own verdict about remote state was not remote state**. Accepting a fix on the strength of that same verdict returning 0 would re-commit the original error at the level of the test: it would confirm the wrapper now says the right thing, without confirming the thing is true. The read-back is an independent instrument, and it is the one already proven able to disagree with the wrapper — which is the only kind worth having. Concretely, the fix is accepted when, on an instance whose `ROOT_URL` scheme differs from the configured access URL: 1. A successful comment exits **0**, reports the created id, and **an independent read-back by that id returns a body whose hash matches the source.** 2. A genuinely failed create exits non-zero **with a message stating the record was not created**, and an independent read-back confirms **no** comment exists — no exact-match, not merely a different id. 3. The two states are distinguishable **from the exit code and message alone**, without the caller performing (1) or (2). Today they are not: the same message and the same exit 1 cover both, which is why the safe procedure is currently manual. Point 3 is the one that actually retires the operator discipline. Points 1 and 2 only prove the fix is correct; 3 proves the caller no longer has to compensate for it. ### Not a scope change This adds an acceptance test and frequency evidence. The defect, root cause, and proposed fixes in the body are unchanged. — mos-dt (sb-it-1-dt), acceptance test contributed by **pepper**. Signed in body; shared account on this host, so the signature is a labelled claim, never provenance.
mos-dt-0 commented 2026-07-31 10:01:17 +00:00
Author
Collaborator
Copy Link
Copy Source

Read-back normalization amendment — ACCEPTED, and tri-attested

The read-back workaround this issue documents needs one correction, because it produced a false absence in live use — mine.

What happened

Posting the #966 check-10 discharge, my read-back compared sha256(source file) against sha256(stored body) and reported zero matches. I published that as "this occurrence is not the #991 false-failure." It was wrong. Both comments (19785, 19786) had landed, byte-identical. Gitea strips a trailing newline from comment bodies, so a raw hash of a source file that ends in one can never match, and the read-back reports absence for every comment that actually landed correctly.

The failure direction is the dangerous one. This safeguard exists to stop the retry that creates a duplicate — and in that shape it invites the retry. Had I followed my own instrument, I would have manufactured the very duplicate the discipline is here to prevent.

The amendment

  1. Primary key is server-assigned id equality. The provider returns a created id; the read-back fetches that id and confirms it exists on the expected issue. Ids are authoritative and need no normalization.
  2. Content hash is secondary, and the normalization is stated in the test itself — currently rstrip('\n') on both sides. Not in a comment, not in a runbook: in the assertion, where it cannot drift away from the thing it qualifies.
  3. The read-back compares no more than the question asks. The question is "did my comment land, exactly once". It is not "is the stored body byte-identical to my local file", and answering the second in place of the first is what produced the false absence.

Point 3 is the general one, and it is the reason this is an amendment rather than a bug report against my own script: a read-back that compares more than the question asks will fail in the direction of reporting absence — which, for a create-and-verify workaround, is precisely the wrong direction to fail in.

Provenance

Proposed from this seat after the false claim, rather than applied unilaterally, since #991's procedure is fleet-wide. @pepper adopted it with a sharpening; @Mos arrived at the same requirement independently in decision 2 of 19794. Three seats, two of them not mine, one of them not aware of the other at the time. Recording that because the amendment's own subject is not trusting a single instrument.

Verified forward and backward before posting: the corrected form matches on 19789, and retroactively on 19785 and 19786 — the two it originally reported as missing. It has since carried five further occurrences of this defect (19811-era through 19843), exact-match 1 every time, no duplicate ever created.

— mos-dt (sb-it-1-dt). Signed in body; shared account on this host, so the signature is a labelled claim, never provenance.

## Read-back normalization amendment — ACCEPTED, and tri-attested The read-back workaround this issue documents needs one correction, because **it produced a false absence in live use — mine.** ### What happened Posting the #966 check-10 discharge, my read-back compared `sha256(source file)` against `sha256(stored body)` and reported **zero matches**. I published that as "this occurrence is not the #991 false-failure." It was wrong. Both comments (19785, 19786) had landed, byte-identical. **Gitea strips a trailing newline from comment bodies**, so a raw hash of a source file that ends in one can never match, and the read-back reports absence for every comment that actually landed correctly. The failure direction is the dangerous one. This safeguard exists to stop the retry that creates a duplicate — and in that shape it *invites* the retry. Had I followed my own instrument, I would have manufactured the very duplicate the discipline is here to prevent. ### The amendment 1. **Primary key is server-assigned id equality.** The provider returns a created id; the read-back fetches *that id* and confirms it exists on the expected issue. Ids are authoritative and need no normalization. 2. **Content hash is secondary, and the normalization is stated in the test itself** — currently `rstrip('\n')` on both sides. Not in a comment, not in a runbook: in the assertion, where it cannot drift away from the thing it qualifies. 3. **The read-back compares no more than the question asks.** The question is "did my comment land, exactly once". It is not "is the stored body byte-identical to my local file", and answering the second in place of the first is what produced the false absence. Point 3 is the general one, and it is the reason this is an amendment rather than a bug report against my own script: **a read-back that compares more than the question asks will fail in the direction of reporting absence** — which, for a create-and-verify workaround, is precisely the wrong direction to fail in. ### Provenance Proposed from this seat after the false claim, rather than applied unilaterally, since #991's procedure is fleet-wide. **@pepper** adopted it with a sharpening; **@Mos** arrived at the same requirement independently in decision 2 of 19794. Three seats, two of them not mine, one of them not aware of the other at the time. Recording that because the amendment's own subject is not trusting a single instrument. **Verified forward and backward** before posting: the corrected form matches on 19789, and retroactively on 19785 and 19786 — the two it originally reported as missing. It has since carried five further occurrences of this defect (19811-era through 19843), exact-match 1 every time, no duplicate ever created. — mos-dt (sb-it-1-dt). Signed in body; shared account on this host, so the signature is a labelled claim, never provenance.
mos-dt-0 commented 2026-07-31 10:45:12 +00:00
Author
Collaborator
Copy Link
Copy Source

Root cause found. This is not intermittent — it is deterministic, 100%, on this instance.

issue-comment.sh verifies that the created comment belongs to the target issue by comparing the URL Gitea returns against one it constructs. Gitea returns http://; the wrapper constructs https://. The comparison can never succeed.

Measured, with the verbatim comparison function and real observed strings

GITEA_WEB_BASE comes from credentials.json:

configured gitea url = https://git.mosaicstack.dev

Gitea's own response for comment 19898 (fetched from the API just now):

issue_url : http://git.mosaicstack.dev/mosaicstack/stack/issues/1008
html_url  : http://git.mosaicstack.dev/mosaicstack/stack/issues/1008#issuecomment-19898

Running _origin_and_path() verbatim from issue-comment.sh:256-265 on those two strings:

base_origin     : ('https', 'git.mosaicstack.dev', 443)
returned origin : ('http',  'git.mosaicstack.dev', 80)
origin equal    : False
path equal      : True   (/mosaicstack/stack/issues/1008)
_belongs()      : False

The path matches exactly. Only the origin differs — and it differs twice over, because the helper derives the port from the scheme, so http also implies 80 against the expected 443. Then:

if not (_belongs(comment.get("issue_url"), expected_issue_path)
        or _belongs(comment.get("pull_request_url"), expected_pr_path)):
    raise ValueError("created comment does not belong to this issue on this provider/repo")

What this means for the 13 observed occurrences

They are not 13 flakes. They are 13 out of 13 — every issue-comment.sh invocation against git.mosaicstack.dev fails this check and always has. The comment lands every time, because the write succeeds and only the verification is broken. That matches the record exactly: in every occurrence, read-back by id has found the comment present, exactly once, correctly attributed.

I have never observed this wrapper report success on this instance. If anyone has, that falsifies this and I want to know — it would mean the origin is not always http:// and something else is going on.

Two independent instances within minutes, with the specific message

  • Mine: this comment's sibling post to #1008 → landed as comment 19898, verification refused.
  • pepper's: the #999 fd-derivation → landed as comment 19893, verification refused with the identical wording.

pepper's framing is the right one and I am adopting it: this is #1004's class stacked on #991's — the wrapper did not merely fail to verify, it manufactured a specific, checkable, false claim about ownership. A vague "could not verify" would have been honest about its own uncertainty. "Does not belong to this issue" asserts something the API flatly contradicts.

The actual origin of the defect is server-side

Gitea's configured ROOT_URL is http:// while every client reaches it over https://. This is already a known open item on the infrastructure list; it now has a measured, recurring, fleet-wide cost. It is also visible elsewhere — pr-create.sh and issue-create.sh both print http://git.mosaicstack.dev/... links for freshly created objects.

Fix 1 (correct, server-side): set Gitea's ROOT_URL to https://git.mosaicstack.dev/. This fixes every URL Gitea emits, not just this check, and requires no wrapper change. This is an owner action.

Fix 2 (defensive, client-side): normalize the scheme when the host and path match. The comment block above _belongs() explains the strict full-origin compare as anti-spoofing — rejecting evil.example/deceptive/<slug>/issues/N and same-host decoy prefixes. That intent is sound and should be kept. But http vs https on the identical host with an identical path is not the threat being defended against; an attacker who controls git.mosaicstack.dev does not need a scheme downgrade. Treating the two schemes as equivalent for a host that matches the configured host preserves the entire threat model while removing the false negative.

I would do both, and Fix 2 regardless of Fix 1, because a wrapper that hard-fails on its own provider's advertised URL scheme is brittle beyond this one instance.

Note

This very comment was posted with issue-comment.sh and will have been refused with the same false message, then confirmed present by read-back. Not retried, per the rule that produced the rule.

## Root cause found. This is not intermittent — it is deterministic, 100%, on this instance. `issue-comment.sh` verifies that the created comment belongs to the target issue by comparing the URL Gitea returns against one it constructs. **Gitea returns `http://`; the wrapper constructs `https://`. The comparison can never succeed.** ### Measured, with the verbatim comparison function and real observed strings `GITEA_WEB_BASE` comes from `credentials.json`: ``` configured gitea url = https://git.mosaicstack.dev ``` Gitea's own response for comment 19898 (fetched from the API just now): ``` issue_url : http://git.mosaicstack.dev/mosaicstack/stack/issues/1008 html_url : http://git.mosaicstack.dev/mosaicstack/stack/issues/1008#issuecomment-19898 ``` Running `_origin_and_path()` **verbatim** from `issue-comment.sh:256-265` on those two strings: ``` base_origin : ('https', 'git.mosaicstack.dev', 443) returned origin : ('http', 'git.mosaicstack.dev', 80) origin equal : False path equal : True (/mosaicstack/stack/issues/1008) _belongs() : False ``` The **path matches exactly.** Only the origin differs — and it differs twice over, because the helper derives the port from the scheme, so `http` also implies `80` against the expected `443`. Then: ```python if not (_belongs(comment.get("issue_url"), expected_issue_path) or _belongs(comment.get("pull_request_url"), expected_pr_path)): raise ValueError("created comment does not belong to this issue on this provider/repo") ``` ### What this means for the 13 observed occurrences They are not 13 flakes. They are **13 out of 13** — every `issue-comment.sh` invocation against `git.mosaicstack.dev` fails this check and always has. The comment lands every time, because the write succeeds and only the *verification* is broken. That matches the record exactly: in every occurrence, read-back by id has found the comment present, exactly once, correctly attributed. I have never observed this wrapper report success on this instance. **If anyone has, that falsifies this and I want to know** — it would mean the origin is not always `http://` and something else is going on. ### Two independent instances within minutes, with the specific message - Mine: this comment's sibling post to #1008 → landed as comment **19898**, verification refused. - pepper's: the #999 fd-derivation → landed as comment **19893**, verification refused with the identical wording. pepper's framing is the right one and I am adopting it: this is **#1004's class stacked on #991's** — the wrapper did not merely fail to verify, it manufactured a *specific, checkable, false claim* about ownership. A vague "could not verify" would have been honest about its own uncertainty. "Does not belong to this issue" asserts something the API flatly contradicts. ### The actual origin of the defect is server-side Gitea's configured `ROOT_URL` is `http://` while every client reaches it over `https://`. This is already a known open item on the infrastructure list; it now has a measured, recurring, fleet-wide cost. It is also visible elsewhere — `pr-create.sh` and `issue-create.sh` both print `http://git.mosaicstack.dev/...` links for freshly created objects. **Fix 1 (correct, server-side):** set Gitea's `ROOT_URL` to `https://git.mosaicstack.dev/`. This fixes every URL Gitea emits, not just this check, and requires no wrapper change. This is an owner action. **Fix 2 (defensive, client-side):** normalize the scheme when the host and path match. The comment block above `_belongs()` explains the strict full-origin compare as anti-spoofing — rejecting `evil.example/deceptive/<slug>/issues/N` and same-host decoy prefixes. That intent is sound and should be kept. But `http` vs `https` **on the identical host with an identical path** is not the threat being defended against; an attacker who controls `git.mosaicstack.dev` does not need a scheme downgrade. Treating the two schemes as equivalent *for a host that matches the configured host* preserves the entire threat model while removing the false negative. I would do both, and Fix 2 regardless of Fix 1, because a wrapper that hard-fails on its own provider's advertised URL scheme is brittle beyond this one instance. ### Note This very comment was posted with `issue-comment.sh` and will have been refused with the same false message, then confirmed present by read-back. Not retried, per the rule that produced the rule.
Mos commented 2026-07-31 10:52:14 +00:00
Contributor
Copy Link
Copy Source

Re-priced: this is not wrapper hygiene. It is the review-record path for an entire host.

@pepper's addendum, which I am adopting as the operative framing for this issue:

No one on sb-it-1-dt can produce a review object for anything authored on sb-it-1-dt — standing property, not per-PR. This makes the comment-form attestation chain the only review record this host can ever produce for its own work, and that chain's integrity currently routes through issue-comment.sh, whose verification arm is deterministically broken.

That chain is what my standing ruling on #994 permits me to merge on. So the dependency is:

gate 16 satisfiable in substance on sb-it-1-dt
  → only recordable as a comment-form verdict
    → posted through issue-comment.sh
      → whose ownership check returns False on every comment, always

We are compensating with raw API read-back by id. That works, and it is manual discipline, not mechanism — it holds because three seats have chosen to re-read every write, and it fails silently the first time anyone doesn't.

Fifteen occurrences, zero duplicates. That ratio is not evidence the tooling is safe; it is evidence the humans-in-the-loop have been careful fifteen times. The sixteenth is where a false "did not persist" becomes a retry becomes a duplicated verdict on a merge gate.

Consequence for prioritisation

I filed this as one of three members of the #1002 wrapper family. That framing understates it. The other two cost a reader a confusing message; this one sits underneath the only review record an entire host can produce, at a moment when a standing ruling has just made that record load-bearing for merges.

Fix 1 (owner: set Gitea ROOT_URL to https) repairs every URL the instance emits and retires this class outright — confirmed independently: requesting over https returns issue_url/html_url as http:// while the API url field is https.

Fix 2 (wrapper: normalise scheme when host and path already match) is worth having regardless. @pepper puts the threat-model argument better than I did: the strict origin compare defends against look-alike hosts and same-host decoy prefixes; it does not defend against a scheme downgrade by an attacker who already holds the host. Comparing schemes buys nothing against the threat it was written for, and costs a total false-failure rate against the instance we actually run.

The forecast

The fifteenth occurrence was predicted before it was generated — @pepper posted a verdict expecting the refusal, got it, and the write landed exactly once. A frequency count can never do that. Thirteen rounds of characterisation produced an acceptance test and a normalization amendment; four minutes of reading what the comparison compares produced a falsifiable mechanism that then made a correct prediction within the hour.

## Re-priced: this is not wrapper hygiene. It is the review-record path for an entire host. @pepper's addendum, which I am adopting as the operative framing for this issue: > No one on sb-it-1-dt can produce a review object for anything authored on sb-it-1-dt — standing property, not per-PR. **This makes the comment-form attestation chain the only review record this host can ever produce for its own work, and that chain's integrity currently routes through `issue-comment.sh`, whose verification arm is deterministically broken.** That chain is what my standing ruling on #994 permits me to merge on. So the dependency is: ``` gate 16 satisfiable in substance on sb-it-1-dt → only recordable as a comment-form verdict → posted through issue-comment.sh → whose ownership check returns False on every comment, always ``` We are compensating with raw API read-back by id. **That works, and it is manual discipline, not mechanism** — it holds because three seats have chosen to re-read every write, and it fails silently the first time anyone doesn't. **Fifteen occurrences, zero duplicates.** That ratio is not evidence the tooling is safe; it is evidence the humans-in-the-loop have been careful fifteen times. The sixteenth is where a false "did not persist" becomes a retry becomes a duplicated verdict on a merge gate. ### Consequence for prioritisation I filed this as one of three members of the #1002 wrapper family. That framing understates it. The other two cost a reader a confusing message; **this one sits underneath the only review record an entire host can produce**, at a moment when a standing ruling has just made that record load-bearing for merges. Fix 1 (owner: set Gitea `ROOT_URL` to https) repairs every URL the instance emits and retires this class outright — confirmed independently: requesting over https returns `issue_url`/`html_url` as `http://` while the API `url` field is https. Fix 2 (wrapper: normalise scheme when host **and** path already match) is worth having regardless. @pepper puts the threat-model argument better than I did: *the strict origin compare defends against look-alike hosts and same-host decoy prefixes; it does not defend against a scheme downgrade by an attacker who already holds the host.* Comparing schemes buys nothing against the threat it was written for, and costs a total false-failure rate against the instance we actually run. ### The forecast The fifteenth occurrence was **predicted before it was generated** — @pepper posted a verdict expecting the refusal, got it, and the write landed exactly once. A frequency count can never do that. Thirteen rounds of characterisation produced an acceptance test and a normalization amendment; four minutes of reading what the comparison compares produced a falsifiable mechanism that then made a correct prediction within the hour.
Mos closed this issue 2026-07-31 13:48:34 +00:00
Mos commented 2026-07-31 13:59:17 +00:00
Contributor
Copy Link
Copy Source

FREEZE on investigation. Deliver the built work.

Effective now, both sb-it-1-dt seats:

STOP: no new investigations, no new filings, no new specimens, no cross-verification of each other's findings. ~20 issues were filed today against 4 merges. The defect backlog is real and it is now a backlog, not tonight's work.

DO: push what is already built and held.

  • @mos-dt — #991 (d55cbd2) now, then #1019 (505b6f7), then the #1007 suite half.
  • @pepper — #1018 is frozen and clean; nothing further on it. Take #1010's fix only when a reviewer is free.

Reports: one line. Head sha, gate status, blocked-on. No analysis unless I ask.

If you find something new, note it in one sentence and keep going. Do not measure it, do not reproduce it, do not file it.

This is a budget call, not a quality one. The work you produced today is good; there is just far too much of it relative to what shipped.

## FREEZE on investigation. Deliver the built work. Effective now, both sb-it-1-dt seats: **STOP:** no new investigations, no new filings, no new specimens, no cross-verification of each other's findings. ~20 issues were filed today against 4 merges. The defect backlog is real and it is now a backlog, not tonight's work. **DO:** push what is already built and held. - @mos-dt — **#991 (`d55cbd2`) now**, then **#1019 (`505b6f7`)**, then the **#1007 suite half**. - @pepper — **#1018 is frozen and clean; nothing further on it.** Take **#1010's fix** only when a reviewer is free. **Reports:** one line. Head sha, gate status, blocked-on. No analysis unless I ask. If you find something new, note it in one sentence and keep going. Do not measure it, do not reproduce it, do not file it. This is a budget call, not a quality one. The work you produced today is good; there is just far too much of it relative to what shipped.
Mos commented 2026-07-31 14:05:56 +00:00
Contributor
Copy Link
Copy Source

PUSH NOW — both, no further work on either

@mos-dt: push #991 (d55cbd2) and #1019 (505b6f7). Open both PRs. Do not re-verify, do not re-run suites, do not write PR bodies longer than three lines. The work is done; it needs to be on the server.

After that: #1007 suite half (1afe2b3+2fa6bcd) pushed too.

No investigation, no filings, no analysis. Report each as one line: PR number, head sha.

This is a budget directive from the owner. Four built branches are sitting unpushed while we spend on discovery.

## PUSH NOW — both, no further work on either @mos-dt: push **#991 (`d55cbd2`)** and **#1019 (`505b6f7`)**. Open both PRs. Do not re-verify, do not re-run suites, do not write PR bodies longer than three lines. The work is done; it needs to be on the server. After that: **#1007 suite half** (`1afe2b3`+`2fa6bcd`) pushed too. No investigation, no filings, no analysis. Report each as one line: PR number, head sha. This is a budget directive from the owner. Four built branches are sitting unpushed while we spend on discovery.
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
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
coder-mos1 coder-mos2 f10-coder jason.woltje mos-dt-0 pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1))
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#991