fix: builder plugin deep-review — auth, backend, adapter, install-script hardening by iap · Pull Request #97 · iap/builder

iap · 2026-08-21T17:23:20Z

Builder Plugin — Review & Fixes (PR Summary)

Deep review of the Hermes builder plugin (Amazon Q / AWS Builder ID integration) found
and fixed 5 high, 11 medium, and 8 low severity issues across the SSO-OIDC device
flow, the chat backend (Amazon Q event-stream parsing), the HTTP adapter, and the
install/uninstall shell scripts.

Test status: 204 passed, 1 skipped (the single skip is the live OIDC test).

Commits (9, on `main` over `ffb4ba3`)

Commit	Scope
`2220221`	Auth refresh hardening, adapter framing, provider ownership (H2–H5, M5, M8, M9, M11, M12)
`92760a4`	Preserve config comments in install/uninstall scripts (H1, M14, L3)
`ad2711a`	Parse AWS event-stream frames properly (M4)
`fc89bbf`	Force UTF-8 output in install scripts (Unicode portability)
`b21244e`	Harden chat backend + adapter robustness (M2, M3, M6, M7)
`19ca324`	Coordinate device-flow polling (M13)
`e9ae2af`	Case-insensitive models, catalog reload, terminal refresh, status exit code (L2, L6, L8, L10)
`73286d0`	Strengthen secret-leak gate, drop redundant `serve_forever` (L5, L9)
`7b86d23`	Drop deprecated `[tool.uv] dev-dependencies` (L4)

High severity (H1–H5)

H1 — config comment stripping: setup.sh/uninstall.sh rewrote config.yaml via YAML
round-trips, silently deleting user comments. Replaced with line-based, comment-preserving
block surgery.
H2 — dead _dashboard.py: removed a stray root-level file that shadowed the real
dashboard/ package and could break imports.
H3/H4 — SSO-OIDC robustness: device-flow token/registration persistence and edge-case
handling hardened (see also M11/M12).
H5 — CI verify job: added a CI job that runs verify.py so the install-time self-check
is exercised on every push.

Medium severity (M2–M14)

M2 — backend.py HTTP call wrapped with a clear error + 50 MB stream cap.
M3 — refresh-then-retry now fires on any 400/401 (dropped a literal "invalid" gate).
M4 — replaced regex-over-UTF8 with a real application/vnd.amazon.eventstream frame
reader (both CRC32s validated) with a lenient text fallback.
M5 — adapter finish_reason handling corrected.
M6 — adapter validates Host, rejects non-loopback Origin, and refuses wildcard binds
even under the public opt-in.
M7 — XML <tool_call> loop capped at 20; bare/fenced JSON tool calls stripped from content.
M8 — _provider.py urlsplit fixed.
M9 — package __init__.py exports corrected.
M11/M12 — SSO-OIDC registration/token edge cases (with H3/H4).
M13 — background poll thread and get_status() now share slow-down state (no more
doubled request rate / repeated SlowDownException).
M14 — fresh-install path in setup.sh now handles empty providers: {} / [] stubs.

Low severity (L2–L10)

L2 — catalog (list_models/load_tags) reloads plugin.yaml on mtime change.
L3 — removed redundant _QuotedDumper/_num_like (PyYAML already quotes these).
L4 — dropped deprecated [tool.uv] dev-dependencies.
L5 — secret-leak gate upgraded from a 2-key denylist to a case-insensitive recursive key scan.
L6 — build_cli.py status exits non-zero when logged out (scriptable).
L8 — start_login() no longer falsely reports already_authenticated for an expired
token; refresh_token() stops retrying terminal InvalidGrantException 3×.
L9 — adapter.py __main__ no longer calls serve_forever() twice.
L10 — _resolve_model_id is case-insensitive (no more silent downgrade of
"Claude-Sonnet-4.5" to "auto").

Deferred (deliberately skipped)

M15 — single provider owner: net-neutral (would relocate comment-stripping into Hermes
core's save_config(), not fix it).
L1 — shared config loader (micro-opt, now moot after L2 mtime caching).
L7 — setup.sh temp-file cleanup (marginal).
L11 — CI ty/shellcheck jobs (no runtime value).

Push status

All 9 commits are local-only on main. Pushing requires a fork or write access to
https://github.com/iap/builder.git — not yet provided.

Greptile Summary

This change improves Builder configuration cleanup and public adapter binding behavior. Focused executions confirmed that uninstall now preserves unrelated Builder-named entries, nested lists, unrelated empty plugin settings, and custom toolset lists, while still removing compact managed cli entries; the public-bind opt-in also accepts a non-loopback Host while continuing to reject non-loopback Origins. One issue remains: removing Builder from a compact cli list can move another surviving toolset entry out of its cli mapping.

Confidence Score: 4/5

Not ready to merge until compact YAML toolset cleanup preserves the cli parent for surviving entries.

A verified non-security configuration corruption issue remains in the uninstall path.

Files Needing Attention: scripts/uninstall.sh

T-Rex Logs

What T-Rex did

To document the uninstall, T-Rex produced an exact compact YAML uninstall test script source and captured configuration snapshots before and after the builder uninstall.
A focused temporary-HERMES_HOME uninstall test was run against five configurations and all assertions passed.
T-Rex produced proof for a posted P1 finding.
Validation showed that before execution, builder and cli were nested under cli:, and after execution, builder and cli were removed while ask_q remained under platform_toolsets and known_plugin_toolsets; the test also enumerated multiple FIXED_OR_INVALID scenarios and how current code paths handle indentation and item removal.
Focused endpoint execution completed with exit code 0, and an additional repository test attempt was blocked due to a missing requests dependency.

_{Ran code and verified through T-Rex}

Comments Outside Diff (5)

General comment

Uninstall removes user-owned nested builder list items in broadly matched paths
- Bug
  - An isolated config placed unrelated - builder values in plugins.enabled.user_groups, platform_toolsets.cli.user_groups, and known_plugin_toolsets.cli.user_groups. Running the actual uninstall script removed all three and pruned their containing branches. Control entries in plugins.disabled and an unrelated top-level list survived.
- Cause
  - scripts/uninstall.sh:114 removes any builder item whose ancestor stack contains enabled beneath top-level plugins; scripts/uninstall.sh:118 removes any builder item under either top-level toolset key. Neither condition requires the expected direct list path.
- Fix
  - Track and require exact supported list paths before deletion (for example, direct plugins.enabled and the intended direct toolset lists), rather than matching arbitrary descendants by a shared ancestor.
_{Ran code and verified through T-Rex}
General comment

Uninstall deletes unrelated empty containers beneath managed top-level sections
- Bug
  - With an isolated config containing plugins.enabled: [- builder] and unrelated plugins.user_groups: [], the real uninstall script removes both the owned Builder entries and the user-owned empty user_groups container, leaving an empty config. The expected user_groups: [] line is absent in the after capture.
- Cause
  - _prune_empty treats every nested line under top-level plugins, providers, platform_toolsets, known_plugin_toolsets, or model as managed (scripts/uninstall.sh:153,168). It then removes any empty literal (scripts/uninstall.sh:172) even when that literal was pre-existing and unrelated to the Builder removal.
- Fix
  - Restrict pruning to containers that became empty due to a Builder-owned removal, or track exact managed paths eligible for pruning (for example plugins.enabled, direct toolset list paths, and removed provider/model containers). Do not prune arbitrary nested [] or {} values merely because their top-level ancestor is managed.
_{Ran code and verified through T-Rex}
General comment

Uninstall removes user-owned direct custom toolset lists
- Bug
  - The real uninstall invocation removed both platform_toolsets.extra: [- builder] and known_plugin_toolsets.custom: [- builder] from a temporary user configuration.
- Cause
  - The two-level predicate at scripts/uninstall.sh:124-128 treats every child toolset name as managed, and pruning removes the now-empty mappings.
- Fix
  - Limit cleanup to the exact installer-managed paths platform_toolsets.cli and known_plugin_toolsets.cli.
_{Ran code and verified through T-Rex}
General comment

Uninstall misses same-indented builder toolset list items
- Bug
  - For valid YAML where cli: and its - builder item share indentation, uninstall reports that no entries were found and leaves the builder entries in both installer-owned toolset paths.
- Cause
  - At scripts/uninstall.sh:88-89, the stack-pop condition uses >= ind. On the same-indented list item it pops the cli ancestor before path is computed at line 96, so the path no longer matches either guarded toolset list at line 125.
- Fix
  - Preserve the mapping-key ancestor for a same-indented sequence item (for example, avoid popping the cli stack entry before list-item path evaluation), while retaining correct stack handling for sibling mapping keys.
_{Ran code and verified through T-Rex}
General comment

Compact YAML pruning re-parents surviving toolsets
- Bug
  - For `cli:
    - builder
    - ask_q, uninstall removes cli:even thoughask_q` survives, placing it beneath the enclosing mapping.
- Cause
  - Lines 201–202 compare a prospective child’s physical indentation to the mapping key indentation, so compact same-column sequence items are not considered children during pruning.
- Fix
  - Use the mapping key’s logical content indentation when identifying children so compact sequence items retain the cli parent.
_{Ran code and verified through T-Rex}

_{Reviews (6): Last reviewed commit: "fix(builder): handle compact same-indent..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

…rship - auth/sso_oidc: treat clientSecretExpiresAt==0 as never-expires (unblock silent refresh); guard token save against logout stop; preserve prior refresh_token; atomic mkstemp secret writes - adapter: emit finish_reason 'stop' on chat-only completion - __init__: guard adapter-port parse, wrap provider registration, set _registered right after tool registration - _provider: urlsplit-based ownership detection (no port-prefix false positive); guard int(env) - ci: verify job checks out hermes-agent + PYTHONPATH so the provider check actually runs - delete dead _dashboard.py; align SSE-shape and api_mode test expectations

H1: setup.sh and uninstall.sh rewrote config.yaml via yaml.safe_load + safe_dump, silently stripping every comment and formatting on each run. - setup.sh: replace the 'already installed' provider refresh and the plugins.enabled step with line-based surgery that preserves comments and unrelated keys verbatim. - setup.sh: fix empty / {} providers handling (M14) - the old detection crashed with 'aws-builder in None' on an empty providers: stub and appended a duplicate providers: key for providers: {}. - setup.sh: drop the redundant _QuotedDumper/_num_like code (L3). - uninstall.sh: line-based removal + atomic write (temp + os.replace) + restore the backup on any failure, and prune now-empty provider/toolset/ model containers like the old yaml path did. Verified: all 5 heredoc bodies py_compile; 17 functional checks pass (comment preservation, provider insert/remove, empty-stub pruning).

Replace the regex scan over a lossy UTF-8 decode of Q's binary application/vnd.amazon.eventstream response with a real frame reader. - _parse_eventstream_frames: parses [total_len][headers_len][prelude_crc] [headers][payload][message_crc] frames (big-endian), validating both CRC32s and header structure. - _parse_eventstream_headers: decodes header name/value pairs (string, bool, and fixed-size numeric value types). - _extract_from_frames: pulls assistant text from payloads carrying both content and modelId, plus conversationId / toolUseId / error __type. - _extract_answer_with_conversation_id: tries framing first, falls back to the existing lenient text path for bare JSON (offline tests). Adds 6 tests: CRC-valid frames, multi-frame concatenation, split-across- chunks, framed error event, conversation/tool id extraction, and CRC- corruption fallback. Full suite: 193 passed, 1 skipped.

The setup.sh/uninstall.sh Python heredocs print ✓/→/✗ glyphs, which crash with UnicodeEncodeError when stdout is a non-UTF-8 pipe (e.g. Windows cp1252 under redirect). Export PYTHONUTF8=1 so the heredocs always emit UTF-8 regardless of locale.

… M7) M2 (backend): wrap requests.post in try/except so Timeout/ConnectionError surface as a clear RuntimeError instead of a raw requests exception; cap the buffered response at 50 MB so a hung/malicious stream can't exhaust memory. M3 (backend): broaden the silent refresh-then-retry to any 400/401 (was gated on a literal 'invalid' substring that missed other auth-failure wording). M6 (adapter): validate Host and reject non-loopback Origin on the token proxy; refuse wildcard binds even under AWS_BUILD_ADAPTER_ALLOW_PUBLIC and warn on any non-loopback bind. M7 (adapter): cap the XML tool-call loop at 20 (was uncapped, only the JSON fallback was capped); strip bare/fenced JSON tool calls from assistant content (previously only XML was stripped, leaking raw JSON to the user).

The background poll thread and get_status() both called _poll_once() on the same flow without sharing slow-down state: the thread kept a local interval snapshot and never re-read the persisted flow, so a SlowDownException bump made by get_status() (same or another process) was ignored — doubling the request rate and causing repeated SlowDownException. - _poll_loop now re-reads the persisted interval each pass (monotonic max), honouring slow_down bumps from any poller. - get_status() skips its manual poll when a live background thread is already driving the flow, avoiding same-process double-polling.

…sh, status exit code Addresses low-severity but real behavioral issues: - backend._resolve_model_id: accept case-variants of catalog models and return the canonical lowercase form instead of silently downgrading to 'auto' (L10). - backend.list_models/load_tags: reload the plugin.yaml override when its mtime changes, making the 'editing plugin.yaml is picked up on the next call' docstring true again (L2). - auth/sso_oidc.start_login: only short-circuit when the token is actually valid (or refreshable), not merely present — an expired non-refreshable token now starts a fresh flow (L8). - auth/sso_oidc.refresh_token: stop retrying terminal InvalidGrantException 3x (dead refresh token); still retry transient ClientError/network errors (L8). - build_cli.cmd_status: exit non-zero (1) when not authenticated, so 'logged out' is scriptable (L6).

…er (L5, L9) - verify.py: replace the two-key snake_case denylist with a case-insensitive, recursive scan for secret-named JSON keys (catches accessToken/clientSecret/ refreshToken/password/etc., without false-positiveing on token_type or has_refresh_token) (L5). - adapter.py __main__: start() already runs serve_forever() in a daemon thread; the main thread now waits on an Event instead of launching a second redundant serve loop (L9).

uv deprecated [tool.uv] dev-dependencies in favor of [project.optional-dependencies] / dependency-groups. The block duplicated the 'dev' extra verbatim and triggered a deprecation warning on every uv run.

…d uninstall, lint - adapter.py: the M6 Host guard now respects AWS_BUILD_ADAPTER_ALLOW_PUBLIC=1, so a non-loopback Host is only rejected when the operator has NOT opted into public binding (the opt-in was previously unusable — every real request 403'd). Origin protection remains in both modes. - scripts/uninstall.sh: replace raw text/indentation matching with a path-scoped cleanup pass, so an unrelated user key/list that merely shares the name 'builder' is no longer removed. - tests: fix UP031 lint (percent-format -> f-strings) and add a regression test for the public-bind Host guard.

Greptile re-review: the ancestor-based - builder match was still too broad. It removed any such item below a plugins.enabled or toolset *descendant* (e.g. plugins.enabled.user_groups, platform_toolsets.cli.user_groups, known_plugin_toolsets.cli.user_groups) — user-owned lists that merely share the name. Require the exact list paths instead: plugins.enabled, or directly under a toolset sub-key (platform_toolsets.<sub> / known_plugin_toolsets.<sub>). Add tests/test_uninstall.py, which execs the script's own embedded helpers (not a mirror) and covers the happy path plus all scoping regressions, including the three nested-path cases from the review.

CodeQL flags verify.py's secret-leak check because it interpolates the leaked field *names* into the check message, which reaches a print sink (py/clear-text-logging-sensitive-data). The names are metadata, not secret values, so this is a false positive — but it fails the CodeQL check. Report the count of leaked field names instead; secret detection is unchanged.

Greptile re-review round 3: _prune_empty removed every empty container under a managed top-level key, including user-owned ones (e.g. plugins.user_groups: [], platform_toolsets.extra: {}). Track the exact container paths _cleanup emptied (providers, plugins.enabled, platform_toolsets.<sub>, known_plugin_toolsets.<sub>, model) and prune only those, cascading to their empty parents. Unrelated empty containers are preserved. Add a regression test for unrelated empty-container preservation.

Greptile round 4: _cleanup removed '- builder' from any direct child of platform_toolsets / known_plugin_toolsets, deleting user-owned custom toolset lists (e.g. platform_toolsets.extra). Only platform_toolsets.cli and known_plugin_toolsets.cli are installer-owned. Restrict the toolset list match to those exact paths; pruning already follows the emptied-path set so a custom sub-key is never touched. Add a regression test for custom toolset list preservation.

Greptile round 5: in the compact YAML form (cli: followed by - builder at the same column), the indent stack popped the cli ancestor before the sequence item was evaluated, so the builder toolset reference survived uninstall. Treat a block sequence item's content as starting after the '- ' indicator (column+2), so it stays nested under its same-column mapping key. Add a regression test for the compact form.

greptile-apps · 2026-08-21T22:02:20Z

+                    if _indent(nxt) <= ind:
+                        break


Compact toolset sibling is re-parented

When a compact toolset list contains Builder and another entry, such as cli:\n - builder\n - ask_q, removing Builder causes _prune_empty to treat ask_q as outside cli. It removes the cli: mapping and leaves ask_q directly under platform_toolsets or known_plugin_toolsets, changing the surviving toolset's configuration path. Determine children using the mapping key's logical content indentation so compact sequence items remain under cli:.

Artifacts

Exact compact YAML uninstall test script source

This captured source is the exact generated script that builds the temporary config, invokes the real uninstaller, and asserts the malformed resulting nesting; the takeaway is that the reproduction procedure is fully inspectable.

Compact YAML configuration before builder uninstall

This setup-command capture shows `builder` and `ask_q` correctly nested under `cli:` in both affected mappings before uninstall; the takeaway is that the initial configuration has the intended parent-child structure.

Compact YAML configuration after builder uninstall

This real-uninstall capture shows both `cli:` keys absent and each surviving `ask_q` directly under its enclosing toolsets mapping; the takeaway is that uninstall reproduces the claimed re-parenting bug.

_{Ran code and verified through T-Rex}

iap added 9 commits August 21, 2026 02:57

chore(builder): drop deprecated [tool.uv] dev-dependencies (L4)

7b86d23

uv deprecated [tool.uv] dev-dependencies in favor of [project.optional-dependencies] / dependency-groups. The block duplicated the 'dev' extra verbatim and triggered a deprecation warning on every uv run.

github-actions Bot added the bug Something isn't working label Aug 21, 2026