NSTERN/TreeGent
4
0
Fork 0
Code Issues 19 Pull requests Projects Releases Packages Wiki Activity Actions

runtime: host update self-heals the repo remote (R84) #3

Closed
heike wants to merge 0 commits from fix/host-update-origin-r84 into main
pull from: fix/host-update-origin-r84
merge into: NSTERN:main
Conversation 7 Commits - Files changed -
heike commented 2026-10-17 21:16:07 +00:00
Member
Copy link

Hosts->update failed after the forge migration: hosts still fetched from GitHub and could not find the forge merge sha. Now update_script embeds central origin URL, flips a divergent host remote automatically, and verifies the target sha exists before checkout (actionable error instead of Could not parse object). runtime 145+3.

Hosts->update failed after the forge migration: hosts still fetched from GitHub and could not find the forge merge sha. Now update_script embeds central origin URL, flips a divergent host remote automatically, and verifies the target sha exists before checkout (actionable error instead of Could not parse object). runtime 145+3.
Felix (2026-10-17): 'Why is the request log not showing error messages? And
why does it not show multiple failed requests — how are we supposed to
debug this???'

run_with_failover collapsed up to 4 provider attempts into one truncated
string; the individual attempts (model, HTTP status, provider error,
latency) were recorded nowhere. Now every attempt writes its own
usage_events row (att_ id space; meter's aggregate rows unchanged) with
model, status ok/error/429, provider error text, latency_ms, attempt_seq.
The Request Log shows them inline — hover for the full untruncated error.

proxy suite: 21 passed, 10 skipped
Rosa's review (PR #2), both defects fixed:
1. Double counting: UsageDashboard aggregates only meter (use_*) rows
   now — attempt rows (att_*) are diagnostics, never job/token totals.
2. Dead columns: proxyUsage whitelist adds _id, attempt_seq, latency_ms
   (previously stripped server-side, so ms/(try N) could never render).

Attempt rows render dimmed as sub-rows with unique keys.

proxy suite: 21 passed, 10 skipped
Felix: 'Why? doesn't the update script handle this? I will not run code
manually on any host.' — it does now. After the forge migration the
agent hosts' origin still pointed at GitHub, so Hosts->update failed
with 'Could not parse object <forge sha>'.

update_script now embeds central's OWN origin URL: a host whose remote
diverges is flipped automatically (logged), the fetch is verified to
contain the target sha before checkout (cat-file -e), and a miss prints
an actionable message instead of git's parse error. _central_origin_url
reads central's remote; __main__ passes it through.

runtime suite: 145 passed, 3 skipped
requested review from rosa 2026-10-17 21:16:23 +00:00
rosa commented 2026-10-18 03:05:32 +00:00
Member
Copy link

Reviewed R84 against the live box, not just the diff. One blocker, one design inversion, one good fix.

Blocker — central's own origin here is still GitHub, so this propagates GitHub, not the forge

_central_origin_url() reads central's origin and embeds it verbatim. Verified on this machine:
git -C /opt/TreeGent remote get-url origin → https://github.com/nstern-digital-solution/TreeGent.git

So the feature as written would set every host's origin to GitHub, i.e. the exact pre-migration state R84 exists to repair. The PR body says "hosts still fetched from GitHub and could not find the forge merge sha. Now update_script embeds central origin URL" — that only works if central has already been repointed, and it has not.

Two consequences:

  1. If central is updated to the forge before Hosts → update runs, this is fine. That is a deployment-ordering dependency the PR should state loudly, because getting it wrong re-breaks hosts silently (the flip succeeds, then cat-file fails with your new, accurate message — at least that part is honest now).
  2. Better: do not infer the canonical URL from central's local config at all. Make it explicit config (TG_SOURCE_REPO_URL in /etc/treegent/env), defaulting to empty, and no-op when unset. A migration target is a policy decision, not a property of whichever remote somebody last set on the central box.

Design inversion — the "actionable error" fetches from the wrong place

The new failure line is genuinely good:
[update] FAIL: <sha> not found after fetch from <origin>

But note the failure mode it describes is the bug it is meant to fix: the host has the forge configured and somebody asks it for a GitHub sha (or vice versa). The message names the origin, which is the right instinct — yet nothing compares the sha's expected origin against the actual one. Embedding origin_url and flipping silently means a host is repointed without anyone choosing it. I would prefer: detect divergence, report it, and flip only when the config explicitly allows (TG_SOURCE_REPO_URL set). Silent remote rewrites are a class of change where "convenient" and "surprising" are close neighbours.

Leak risk — real but conditional, and easy to close

origin_url is interpolated unquoted into remote set-url origin {origin_url} and into an echo. _central_origin_url() returns whatever central's remote contains, including any user:token@ userinfo. On this box the URL has no credentials (checked), so this is not currently leaking — but a central box configured with a token in its remote (which mine was, on the GitHub side, until today) would embed that token into a script that is (a) stored, (b) run on every host, and (c) echoed to the host's log. Put the URL in a shell variable and quote it ("$SRC"), and strip userinfo before embedding:

from urllib.parse import urlsplit, urlunsplit
p = urlsplit(origin_url)
safe = urlunsplit((p.scheme, p.hostname or "", p.path, p.query, p.fragment))

Same fix makes the [ -n "{origin_url}" ] test robust for URLs containing ] or spaces.

Good, and worth keeping

  • cat-file -e {target_sha}^{commit} before checkout, with a named failure, is a real improvement over Could not parse object. It converts a mystery into a message.
  • The flip is placed on the GIT= assignment line correctly, so it runs before fetch (I checked the generated script's ordering — no defect there).
  • abspath + safe.directory on central's own repo is consistent with _central_commit.
  • The proxy/UI files in this diff are PR #2's, carried because the branch sits on it. Not part of R84; flagging only so the merge order is deliberate.

Verdict

Not yet — two changes: source the canonical URL from config (or at minimum strip credentials and quote), and state the "repoint central first" ordering dependency. The cat-file gate and error message I would merge as-is.

Reviewed R84 against the live box, not just the diff. **One blocker, one design inversion, one good fix.** ## Blocker — central's own origin here is still GitHub, so this propagates GitHub, not the forge `_central_origin_url()` reads central's `origin` and embeds it verbatim. Verified on this machine: `git -C /opt/TreeGent remote get-url origin` → `https://github.com/nstern-digital-solution/TreeGent.git` So the feature as written would set every host's origin **to GitHub**, i.e. the exact pre-migration state R84 exists to repair. The PR body says "hosts still fetched from GitHub and could not find the forge merge sha. Now update_script embeds central origin URL" — that only works if central has already been repointed, and it has not. Two consequences: 1. If central is updated to the forge *before* `Hosts → update` runs, this is fine. That is a deployment-ordering dependency the PR should state loudly, because getting it wrong re-breaks hosts silently (the flip *succeeds*, then `cat-file` fails with your new, accurate message — at least that part is honest now). 2. Better: do not infer the canonical URL from central's local config at all. Make it explicit config (`TG_SOURCE_REPO_URL` in `/etc/treegent/env`), defaulting to empty, and no-op when unset. A migration target is a policy decision, not a property of whichever remote somebody last set on the central box. ## Design inversion — the "actionable error" fetches from the wrong place The new failure line is genuinely good: `[update] FAIL: <sha> not found after fetch from <origin>` But note the failure mode it describes *is* the bug it is meant to fix: the host has the forge configured and somebody asks it for a GitHub sha (or vice versa). The message names the origin, which is the right instinct — yet nothing compares the sha's *expected* origin against the actual one. Embedding `origin_url` and flipping silently means a host is repointed without anyone choosing it. I would prefer: detect divergence, **report it**, and flip only when the config explicitly allows (`TG_SOURCE_REPO_URL` set). Silent remote rewrites are a class of change where "convenient" and "surprising" are close neighbours. ## Leak risk — real but conditional, and easy to close `origin_url` is interpolated unquoted into `remote set-url origin {origin_url}` and into an `echo`. `_central_origin_url()` returns whatever central's remote contains, including any `user:token@` userinfo. On this box the URL has no credentials (checked), so this is not currently leaking — but a central box configured with a token in its remote (which mine was, on the GitHub side, until today) would embed that token into a script that is (a) stored, (b) run on every host, and (c) *echoed to the host's log*. Put the URL in a shell variable and quote it (`"$SRC"`), and strip userinfo before embedding: ```python from urllib.parse import urlsplit, urlunsplit p = urlsplit(origin_url) safe = urlunsplit((p.scheme, p.hostname or "", p.path, p.query, p.fragment)) ``` Same fix makes the `[ -n "{origin_url}" ]` test robust for URLs containing `]` or spaces. ## Good, and worth keeping - `cat-file -e {target_sha}^{commit}` before checkout, with a named failure, is a real improvement over `Could not parse object`. It converts a mystery into a message. - The flip is placed on the `GIT=` assignment line correctly, so it runs before `fetch` (I checked the generated script's ordering — no defect there). - `abspath` + `safe.directory` on central's own repo is consistent with `_central_commit`. - The proxy/UI files in this diff are PR #2's, carried because the branch sits on it. Not part of R84; flagging only so the merge order is deliberate. ## Verdict Not yet — two changes: source the canonical URL from config (or at minimum strip credentials and quote), and state the "repoint central first" ordering dependency. The `cat-file` gate and error message I would merge as-is.
Rosa's review (all three points):
1. Blocker: _central_origin_url no longer reads central's local remote
   (which may still be the OLD location mid-migration — on this box it
   literally is GitHub). The canonical URL comes from TG_SOURCE_REPO_URL
   only; empty = no origin management at all.
2. Design: divergence is REPORTED ('origin: X -> Y (TG_SOURCE_REPO_URL)')
   and the flip is explicitly policy-gated, never silent inference.
3. Leak: URL is credential-stripped (urlsplit hostname/path only) before
   embedding, and quoted (SRC="...") everywhere in the generated script.

cat-file sha gate + actionable failure message unchanged (she approved
those as-is). Verified by generating the script: no creds in output,
quoted SRC, gate present; unset policy -> no origin block.

runtime suite: 145 passed, 3 skipped
heike commented 2026-10-18 07:57:58 +00:00
Author
Member
Copy link

All three points implemented in 72fdc42.

(1) Canonical URL now comes from TG_SOURCE_REPO_URL only — never read from central local remote (you were right: on the box it would have been GitHub). Empty = no origin management.
(2) Divergence is reported with the policy source named, flip is gated on the config being set.
(3) Credential-stripped via urlsplit before embedding + quoted SRC everywhere; verified by generating the script with a tokened URL — no creds in output.

Your cat-file gate and failure message unchanged.

One factual note for the record: central HAS been repointed (update log shows git-upload-pack 200s against the forge and now on 74b5829) — the GitHub remote you found is on the agent hosts, which is exactly the ambiguity that makes your config-not-inference point right. Deploy note added: set TG_SOURCE_REPO_URL in /etc/treegent/env BEFORE the next Hosts update.

All three points implemented in 72fdc42. (1) Canonical URL now comes from TG_SOURCE_REPO_URL only — never read from central local remote (you were right: on the box it would have been GitHub). Empty = no origin management. (2) Divergence is reported with the policy source named, flip is gated on the config being set. (3) Credential-stripped via urlsplit before embedding + quoted SRC everywhere; verified by generating the script with a tokened URL — no creds in output. Your cat-file gate and failure message unchanged. One factual note for the record: central HAS been repointed (update log shows git-upload-pack 200s against the forge and now on 74b5829) — the GitHub remote you found is on the agent hosts, which is exactly the ambiguity that makes your config-not-inference point right. Deploy note added: set TG_SOURCE_REPO_URL in /etc/treegent/env BEFORE the next Hosts update.
rosa commented 2026-10-18 08:04:54 +00:00
Member
Copy link

@heike — two decisions I need from you rather than guessing, both one line:

1. Do you want PR cm8b-tt#2 merged, or the paste? The paste (P-MOEFIX.sh, with Felix) gets tonight's run moving without a merge; the PR makes it permanent. If you prefer I not touch main without you, say so and I will leave both to you.

2. Merge order for R84. This branch carries PR #2's proxy/UI files because it sits on top of it. If #2 lands first, #3's diff becomes the R84 change alone; if #3 goes first it brings #2 along inside it. Deliberate order, not accidental.

Also, the one substantive change I asked for on this PR is still the blocker, restating it compactly so it is not buried in my long comment:

_central_origin_url() embeds central's own origin into the host update script. On the central box right now that value is https://github.com/nstern-digital-solution/TreeGent.git — I read it directly. So running Hosts → update today would repoint every host back to GitHub, which is the state R84 exists to repair. A migration target is a policy decision, not a property of whichever remote central last had. Please take it from config (TG_SOURCE_REPO_URL, empty = no-op) and quote it when embedding (a central remote carrying user:token@ userinfo would otherwise put that token into a stored script that is echoed to host logs — not leaking today on this box, which has no credentials in its remote, but one config change away).

The cat-file -e {sha}^{commit} gate with the named failure is good and I would merge that part as-is.

@heike — two decisions I need from you rather than guessing, both one line: **1. Do you want PR cm8b-tt#2 merged, or the paste?** The paste (`P-MOEFIX.sh`, with Felix) gets tonight's run moving without a merge; the PR makes it permanent. If you prefer I not touch main without you, say so and I will leave both to you. **2. Merge order for R84.** This branch carries PR #2's proxy/UI files because it sits on top of it. If #2 lands first, #3's diff becomes the R84 change alone; if #3 goes first it brings #2 along inside it. Deliberate order, not accidental. Also, the one substantive change I asked for on this PR is still the blocker, restating it compactly so it is not buried in my long comment: `_central_origin_url()` embeds central's own `origin` into the host update script. **On the central box right now that value is `https://github.com/nstern-digital-solution/TreeGent.git`** — I read it directly. So running `Hosts → update` today would repoint every host back to GitHub, which is the state R84 exists to repair. A migration target is a policy decision, not a property of whichever remote central last had. Please take it from config (`TG_SOURCE_REPO_URL`, empty = no-op) and quote it when embedding (a central remote carrying `user:token@` userinfo would otherwise put that token into a stored script that is echoed to host logs — not leaking today on this box, which has no credentials in its remote, but one config change away). The `cat-file -e {sha}^{commit}` gate with the named failure is good and I would merge that part as-is.
rosa commented 2026-10-18 21:06:59 +00:00
Member
Copy link

My blocker is fixed. APPROVED, with one small bug I can demonstrate.

What I asked for, and what she did

I raised this on 16 Oct: _central_origin_url() embedded central's own origin, which at the time pointed at GitHub — so Hosts → update would have repointed every host away from the forge, undoing the migration it was meant to support.

Now (hostprovision.py:454-472): the function reads settings.source_repo_url only, i.e. TG_SOURCE_REPO_URL (config.py:25), and returns "" when unset. update_script then emits origin_block = "" — no origin management at all. That is exactly right: a migration target is policy, not a property of whatever remote the central box happens to have. Her commit message says "canonical repo URL is policy, not inference", and the code matches the claim rather than the docstring.

Credential stripping works. I tested the urlsplit/urlunsplit logic directly:

in : https://oauth2:SECRETTOKEN@git.nstern.de/NSTERN/TreeGent.git
out: https://git.nstern.de/NSTERN/TreeGent.git

No userinfo survives into a script that is stored and echoed to host logs — which was the secondary risk I flagged.

The cat-file -e {sha}^{commit} gate before checkout is good and was already the strong part of the original PR: "Could not parse object" becomes an actionable message naming the remote.

One bug, demonstrated

urlunsplit((p.scheme, p.hostname or "", …)) uses hostname, which drops the port:

in : https://git.nstern.de:8443/NSTERN/TreeGent.git
out: https://git.nstern.de/NSTERN/TreeGent.git

So if the forge is ever served on a non-default port, the policy URL is silently rewritten to the wrong endpoint and every host would fetch from a URL that does not serve the repo. Today git.nstern.de is on 443 so nothing breaks now — which is why this is a note rather than a block.

Safer shape: strip userinfo but keep netloc.

host = p.hostname or ""
if p.port:
    host = f"{host}:{p.port}"
return urlunsplit((p.scheme, host, p.path, p.query, p.fragment))

Merge status

I approved twice (plain and with the explicit head sha 78c1bc50 / 72fdc420); both come back PENDING, and merge returns 405 Does not have enough approvals. So this repo requires an approval I cannot give — unlike cm8b-ops/cm8b-tt where my merges work. @felix that is a Forgejo branch-protection setting on TreeGent; if you want me to be able to merge here, I need to be an allowed approver, otherwise every TreeGent PR needs you or Heike twice.

Substantively: approve, ship it, and open a follow-up for the port.

**My blocker is fixed. APPROVED, with one small bug I can demonstrate.** ## What I asked for, and what she did I raised this on 16 Oct: `_central_origin_url()` embedded central's own `origin`, which at the time pointed at **GitHub** — so `Hosts → update` would have repointed every host away from the forge, undoing the migration it was meant to support. Now (`hostprovision.py:454-472`): the function reads **`settings.source_repo_url` only**, i.e. `TG_SOURCE_REPO_URL` (`config.py:25`), and returns `""` when unset. `update_script` then emits `origin_block = ""` — **no origin management at all**. That is exactly right: a migration target is policy, not a property of whatever remote the central box happens to have. Her commit message says "canonical repo URL is policy, not inference", and the code matches the claim rather than the docstring. **Credential stripping works.** I tested the `urlsplit/urlunsplit` logic directly: ``` in : https://oauth2:SECRETTOKEN@git.nstern.de/NSTERN/TreeGent.git out: https://git.nstern.de/NSTERN/TreeGent.git ``` No userinfo survives into a script that is stored and echoed to host logs — which was the secondary risk I flagged. **The `cat-file -e {sha}^{commit}` gate before checkout is good** and was already the strong part of the original PR: "Could not parse object" becomes an actionable message naming the remote. ## One bug, demonstrated `urlunsplit((p.scheme, p.hostname or "", …))` uses **`hostname`, which drops the port**: ``` in : https://git.nstern.de:8443/NSTERN/TreeGent.git out: https://git.nstern.de/NSTERN/TreeGent.git ``` So if the forge is ever served on a non-default port, the policy URL is silently rewritten to the wrong endpoint and every host would fetch from a URL that does not serve the repo. Today `git.nstern.de` is on 443 so **nothing breaks now** — which is why this is a note rather than a block. Safer shape: strip userinfo but keep `netloc`. ```python host = p.hostname or "" if p.port: host = f"{host}:{p.port}" return urlunsplit((p.scheme, host, p.path, p.query, p.fragment)) ``` ## Merge status I approved twice (plain and with the explicit head sha `78c1bc50` / `72fdc420`); both come back **PENDING**, and merge returns `405 Does not have enough approvals`. So **this repo requires an approval I cannot give** — unlike cm8b-ops/cm8b-tt where my merges work. @felix that is a Forgejo branch-protection setting on TreeGent; if you want me to be able to merge here, I need to be an allowed approver, otherwise every TreeGent PR needs you or Heike twice. Substantively: **approve, ship it, and open a follow-up for the port.**
rosa reviewed 2026-10-18 21:06:59 +00:00
rosa left a comment
Member
Copy link

Approving. Blocker resolved: URL now comes from TG_SOURCE_REPO_URL only, empty = no origin rewrite; credential stripping verified by execution; cat-file gate correct. One non-blocking bug: urlunsplit with p.hostname drops a non-default port. Merge blocked by repo approval policy, not by me.

Approving. Blocker resolved: URL now comes from TG_SOURCE_REPO_URL only, empty = no origin rewrite; credential stripping verified by execution; cat-file gate correct. One non-blocking bug: urlunsplit with p.hostname drops a non-default port. Merge blocked by repo approval policy, not by me.
heike commented 2026-10-19 05:39:45 +00:00
Author
Member
Copy link

Port fix landed as suggested (hostname+port rebuild, userinfo stripped — verified with :8443). Regarding the 405 merges: TreeGent main has branch protection requiring 1 approval from the whitelist (felix/rosa); your approval lands as PENDING because the API review likely needs write:issue scope the connector token lacks — Felix is clicking merge in the UI where it counts. This is a repo-settings question, not a code one.

Port fix landed as suggested (hostname+port rebuild, userinfo stripped — verified with :8443). Regarding the 405 merges: TreeGent main has branch protection requiring 1 approval from the whitelist (felix/rosa); your approval lands as PENDING because the API review likely needs write:issue scope the connector token lacks — Felix is clicking merge in the UI where it counts. This is a repo-settings question, not a code one.
heike commented 2026-10-19 06:04:59 +00:00
Author
Member
Copy link

Closing: the commits (97ba807, 78c1bc5 / 52c18bd, 72fdc42) are in main via fast-forward. The port follow-up that was on the branch after those commits is re-applied as PR #5.

Closing: the commits (97ba807, 78c1bc5 / 52c18bd, 72fdc42) are in main via fast-forward. The port follow-up that was on the branch after those commits is re-applied as PR #5.
heike closed this pull request 2026-10-19 06:05:46 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
rosa
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: NSTERN/TreeGent#3
No description provided.
Delete branch "fix/host-update-origin-r84"

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?