fix(builder): harden provider ownership + setup.sh manifest fallback - #102
Conversation
|
Fixed in a590d13 — the adapter port is now persisted and honored by ownership checks:
Full suite 239 passed, 1 skipped; verify.py and ruff green. |
|
Both P1s fixed in 53f2fd7 by replacing the port-only stamp with a full provider-entry stamp:
Full suite 241 passed, 1 skipped; verify.py, ruff, and shell syntax all green. |
| return False | ||
| base = entry.get("base_url") or "" | ||
| return _is_our_base_url(base) | ||
| return _matches_stamp(entry) or _is_our_base_url(entry.get("base_url") or "") |
There was a problem hiding this comment.
When a stored provider stamp no longer matches providers.aws-builder, this check still adopts the entry if its URL uses localhost port 8088 or the active adapter port. A user can repurpose that slug for another local service while retaining either port; registration then overwrites their provider configuration, and runtime unregistration removes it. Treat a valid stamp mismatch as user-managed instead of falling back to port-based ownership.
What kind of change is this?
Summary
Three hardening fixes so the install/uninstall scripts and the runtime provider-ownership check agree, and neither can clobber a user-managed provider entry:
e7c1184) —PLUGIN_YAMLwas hardcoded to${HERMES_HOME}/plugins/builder/plugin.yaml. Running setup.sh from a source checkout beforehermes plugins installcrashed the block-generation heredoc (FileNotFoundError,set -eabort mid-setup, after the backup). Now: installed copy → source checkout → cleanexit 1with a clear message. New tests run the real script end-to-end (fallback, installed-copy preference, clean failure).5dd89dd) —_provider._is_our_base_url()defaulted a port-less URL to 8088, sohttp://localhost/v1was misclassified as ours:register_providerwould adopt/clobber it andunregister_providerwould delete it. Every writer of our entries (setup.sh, register_provider) emits the port explicitly, so requiring it has no false negatives.299f627) — provider blocks ataws-builder:/builder:are removed only when plugin-owned:base_urlmatches the loopback adapter (host + explicit port, honoringAWS_BUILD_ADAPTER_PORT) or is absent (dangling leftover, matching the script's historical contract). A block with a foreignbase_urlis kept with an informational note, and amodel.providerpointing at a kept entry is preserved (still valid). One pre-scan supplies the verdict to both cleanups, so key order in the file cannot make them disagree.1c33503) — README + docs/installation.md describe the ownership gate.Ownership semantics deliberately differ from runtime
_is_our_entryin exactly one place, documented in-code: uninstall removes abase_url-less block (user-invoked, fresh backup on disk); runtimeunregister_provider()never does (it runs automatically).Branch
This PR was opened from a branch using one of:
fix/…bugs/…feature/…Checklist
python -m pytest -q -k "not adapter"(full suite: 234 passed, 1 skipped — 6 new tests)verify.pyis greenGreptile Summary
This change improves Builder provider ownership handling for custom ports and failed configuration saves. It is not merge-safe until the remaining provider-ownership issue is addressed: a user-managed provider at the Builder slug can still be adopted based solely on its loopback port after it no longer matches the stored ownership record.
Confidence Score: 4/5
Not merge-safe because one existing provider-ownership issue remains unresolved.
The outstanding finding remains valid: when a stored provider record no longer matches
providers.aws-builder, the current fallback still treats an entry on localhost port 8088 or the active adapter port as Builder-owned. A user who repurposes that slug for another local service can therefore have its provider configuration overwritten during registration or removed during unregistration. The custom-port uninstall finding was resolved by greptile-apps[bot] manually without explanation. The stale ownership-stamp finding was resolved by greptile-apps[bot] manually without explanation. The stamp-after-saving finding was resolved by greptile-apps[bot] manually without explanation.Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/harden-scri..." | Re-trigger Greptile