Skip to content

fix(builder): harden provider ownership + setup.sh manifest fallback - #102

Merged
iap merged 7 commits into
mainfrom
fix/harden-scripts-provider-ownership
Sep 6, 2026
Merged

fix(builder): harden provider ownership + setup.sh manifest fallback#102
iap merged 7 commits into
mainfrom
fix/harden-scripts-provider-ownership

Conversation

@iap

@iap iap commented Sep 5, 2026

Copy link
Copy Markdown
Owner

What kind of change is this?

  • bug fix + test
  • security fix

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:

  1. setup.sh manifest fallback (e7c1184) — PLUGIN_YAML was hardcoded to ${HERMES_HOME}/plugins/builder/plugin.yaml. Running setup.sh from a source checkout before hermes plugins install crashed the block-generation heredoc (FileNotFoundError, set -e abort mid-setup, after the backup). Now: installed copy → source checkout → clean exit 1 with a clear message. New tests run the real script end-to-end (fallback, installed-copy preference, clean failure).
  2. Explicit adapter port required (5dd89dd) — _provider._is_our_base_url() defaulted a port-less URL to 8088, so http://localhost/v1 was misclassified as ours: register_provider would adopt/clobber it and unregister_provider would delete it. Every writer of our entries (setup.sh, register_provider) emits the port explicitly, so requiring it has no false negatives.
  3. uninstall.sh ownership gate (299f627) — provider blocks at aws-builder:/builder: are removed only when plugin-owned: base_url matches the loopback adapter (host + explicit port, honoring AWS_BUILD_ADAPTER_PORT) or is absent (dangling leftover, matching the script's historical contract). A block with a foreign base_url is kept with an informational note, and a model.provider pointing 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.
  4. docs (1c33503) — README + docs/installation.md describe the ownership gate.

Ownership semantics deliberately differ from runtime _is_our_entry in exactly one place, documented in-code: uninstall removes a base_url-less block (user-invoked, fresh backup on disk); runtime unregister_provider() never does (it runs automatically).

Branch

This PR was opened from a branch using one of:

  • fix/…
  • bugs/…
  • feature/…

Checklist

  • Tests pass: python -m pytest -q -k "not adapter" (full suite: 234 passed, 1 skipped — 6 new tests)
  • verify.py is green
  • No raw tokens/secrets in code, logs, or tool output
  • Adapter stays loopback-only unless an explicit guard is added (untouched)
  • Updated docs/README if user-facing behavior changed

Greptile 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

@github-actions github-actions Bot added the bug Something isn't working label Sep 5, 2026
Comment thread scripts/uninstall.sh Outdated
@iap

iap commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Fixed in a590d13 — the adapter port is now persisted and honored by ownership checks:

  • setup.sh writes <HERMES_HOME>/builder/adapter_port (after the config update is verified) — the plugin's data dir that survives reinstalls, same reasoning as the token store; deliberately not an extra key in config.yaml, which Hermes core flags as unknown.
  • register_provider() stamps the actual bound port the same way (best-effort, never fails registration), so runtime-written custom ports persist too.
  • uninstall.sh and _provider._is_our_base_url() now match against the union of known ports: env override + persisted stamp + the 8088 default. Your exact scenario (setup at :9999 → uninstall without the env var) now removes the provider entry and its model.provider reference; regression tests cover the stamp write, stamp-honoring uninstall, the foreign-port-without-stamp case, and a real setup→uninstall bash roundtrip at :9999.

Full suite 239 passed, 1 skipped; verify.py and ruff green.

Comment thread _provider.py Outdated
Comment thread _provider.py Outdated
@iap

iap commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Both P1s fixed in 53f2fd7 by replacing the port-only stamp with a full provider-entry stamp:

  • Retire stale ownership stamps — the stamp (<HERMES_HOME>/builder/adapter_stamp.json) now records the entire entry we wrote, not just the port. Ownership requires the current entry to still carry every string field the stamp recorded (name, base_url, api_key, transport, model; models/discover_models are exempt since register rewrites them every load). A user who repurposes the slug for their own service — even on the same port — changes at least one stamped field and is no longer adopted or removed. Regression test: test_provider_register_stamps_provider_entry / test_uninstall_keeps_user_repurposed_entry_at_stamped_port.
  • Stamp after savingregister_provider() now stamps only on the no-op path (entry already live) and after a successful save_config(); a failed save leaves no stamp. Regression test: test_provider_register_failure_does_not_stamp.
  • setup.sh writes the same JSON stamp (parsed from the block it just wrote and verified in config), and uninstall.sh deletes the stamp after removing the entry so a future user-owned entry can't inherit ownership. The port-based rule now only covers the env override and the 8088 default (pre-stamp legacy entries), so a port can never outlive the entry it was recorded for.

Full suite 241 passed, 1 skipped; verify.py, ruff, and shell syntax all green.

Comment thread _provider.py
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 "")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Honor stamp mismatches

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.

T-Rex Ran code and verified through T-Rex

Fix in Cursor

@iap
iap merged commit c94f199 into main Sep 6, 2026
8 checks passed
@iap
iap deleted the fix/harden-scripts-provider-ownership branch September 6, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant