Auth credential removal undermined by automatic re-seeding

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #67429 fix(credentials): pool entry delete via API sticks — suppress source re-seeding (#55217)

  merged to main

  files: hermes_cli/web_server.py, tests/hermes_cli/test_dashboard_admin_endpoints.py

  Show the diff

  • hermes_cli/web_server.py

    @@ -12498,6 +12498,7 @@ async def add_credential_pool_entry(body: CredentialPoolAdd):
             load_pool,
             PooledCredential,
             AUTH_TYPE_API_KEY,
    +        CUSTOM_POOL_PREFIX,
             SOURCE_MANUAL,
         )
     
    @@ -12519,6 +12520,23 @@ async def add_credential_pool_entry(body: CredentialPoolAdd):
                 access_token=[redacted:secret-assignment]
             )
             pool.add_entry(entry)
    +        # Re-adding a credential is an explicit re-engagement signal: lift
    +        # every suppression for this provider so a source deleted earlier
    +        # (via DELETE below or `hermes auth remove`) can seed again.
    +        # Mirrors the `hermes auth add` behaviour in auth_commands.py.
    +        if not provider.startswith(CUSTOM_POOL_PREFIX):
    +            try:
    +                from hermes_cli.auth import (
    +                    _load_auth_store,
    +                    unsuppress_credential_source,
    +                )
    +                suppressed = _load_auth_store().get("suppressed_sources", {})
    +                for src in list(suppressed.get(provider, []) or []):
    +                    unsuppress_credential_source(provider, src)
    +            except Exception:
    +                _log.exception("unsuppress after pool add failed (non-fatal)")
    +    except HTTPException:
    +        raise
         except Exception as exc:
             _log.exception("POST /api/credentials/pool failed")
             raise HTTPException(status_code=400, detail=str(exc)) from exc
    @@ -12527,8 +12545,20 @@ async def add_credential_pool_entry(body: CredentialPoolAdd):
     
     @app.delete("/api/credentials/pool/{provider}/{index}")
     async def remove_credential_pool_entry(provider: str, index: int):
    -    """Remove a pool entry.  ``index`` is 1-based (matches the list response)."""
    +    """Remove a pool entry.  ``index`` is 1-based (matches the list response).
    +
    +    Removal must be sticky (#55217): ``load_pool()`` re-seeds entries from
    +    their backing source (.env var, OAuth singleton file, custom-provider
    +    config) on every call, so deleting only the pool row silently reverts on
    +    the next dashboard refresh.  We dispatch through the same RemovalStep
    +    registry the CLI ``hermes auth remove`` uses: each source cleans up its
    +    external state and suppresses ``(provider, source)`` so the seeders skip
    +    it.  Manual entries have no registered step — nothing external to clean,
    +    no suppression needed (they aren't re-seeded).
    +    """
         from agent.credential_pool import load_pool
    +    from agent.credential_sources import find_removal_step
    +    from hermes_cli.auth import suppress_credential_source
     
         provider = (provider or "").strip().lower()
         try:
    @@ -12539,7 +12569,36 @@ async def remove_credential_pool_entry(provider: str, index: int):
             raise HTTPException(status_code=400, detail=str(exc)) from exc
         if removed is None:
             raise HTTPException(status_code=404, detail="No pool entry at that index")
    -    return {"ok": True, "provider": provider, "count": len(pool.entries())}
    +
    +    cleaned: List[str] = []
    +    hints: List[str] = []
    +    step = find_removal_step(provider, removed.source or "")
    +    if step is not None:
    +        try:
    +            result = step.remove_fn(provider, removed)
    +            cleaned = list(result.cleaned)
    +            hints = list(result.hints)
    +            if result.suppress:
    +                suppress_credential_source(provider, removed.source)
    +        except Exception:
    +            # Cleanup is best-effort, but suppression is the actual bug fix —
    +            # without it the entry resurrects on the next load_pool().  Apply
    +            # it even when source-specific cleanup blew up.
    +            _log.exception(
    +                "credential source cleanup failed for %s/%s; suppressing anyway",
    +                provider, removed.source,
    +            )
    +            try:
    +                suppress_credential_source(provider, removed.source)
    +            except Exception:
    +                _log.exception("suppress_credential_source failed")
    +    return {
    +        "ok": True,
    +        "provider": provider,
    +        "count": len(pool.entries()),
    +        "cleaned": cleaned,
    +        "hints": hints,
    +    }
     
     
     # ---------------------------------------------------------------------------

  • tests/hermes_cli/test_dashboard_admin_endpoints.py

    @@ -253,6 +253,104 @@ def test_empty_body_rejected(self):
             )
             assert r.status_code == 400
     
    +    def test_env_seeded_delete_stays_deleted(self):
    +        """#55217: DELETE must suppress the source or load_pool() resurrects it.
    +
    +        load_pool() re-seeds from ~/.hermes/.env on every call, so removing
    +        just the pool row silently reverts on the next dashboard refresh.
    +        The endpoint must mirror `hermes auth remove`: clean up the backing
    +        source and suppress (provider, source).
    +        """
    +        from agent.credential_pool import load_pool
    +        from hermes_cli.auth import is_source_suppressed
    +        from hermes_cli.config import save_env_value
    +
    +        fake_key = "sk-or-" + "x" * 20  # constructed, never a real key shape
    +        save_env_value("OPENROUTER_API_KEY", fake_key)
    +
    +        entries = load_pool("openrouter").entries()
    +        assert [e.source for e in entries] == ["env:OPENROUTER_API_KEY"]
    +
    +        r = self.client.delete("/api/credentials/pool/openrouter/1")
    +        assert r.status_code == 200
    +
    +        # Suppressed exactly like the CLI removal path.
    +        assert is_source_suppressed("openrouter", "env:OPENROUTER_API_KEY")
    +
    +        # Even if the backing var comes back (shell export, another process
    +        # rewriting .env), the removal must stay sticky.
    +        save_env_value("OPENROUTER_API_KEY", fake_key)
    +        assert load_pool("openrouter").entries() == []
    +        assert self.client.get("/api/credentials/pool").json()["providers"] == []
    +
    +    def test_post_readd_lifts_suppression(self):
    +        """Re-adding via POST is an explicit re-engagement — suppressions lift.
    +
    +        Mirrors `hermes auth add`, which clears every suppression for the
    +        provider so a user who deleted a credential and re-adds one isn't
    +        silently blocked from env re-seeding.
    +        """
    +        from agent.credential_pool import load_pool
    +        from hermes_cli.auth import is_source_suppressed
    +        from hermes_cli.config import save_env_value
    +
    +        fake_key = "sk-or-" + "y" * 20
    +        save_env_value("OPENROUTER_API_KEY", fake_key)
    +        load_pool("openrouter")
    +        assert self.client.delete("/api/credentials/pool/openrouter/1").status_code == 200
    +        assert is_source_suppressed("openrouter", "env:OPENROUTER_API_KEY")
    +
    +        r = self.client.post(
    +            "/api/credentials/pool",
    +            json={"provider": "openrouter", "api_key": "[redacted:secret-assignment]" + "z" * 20},
    +        )
    +        assert r.status_code == 200
    +        assert not is_source_suppressed("openrouter", "env:OPENROUTER_API_KEY")
    +
    +        # Key back in .env + suppression lifted → env entry seeds alongside
    +        # the manual one.
    +        save_env_value("OPENROUTER_API_KEY", fake_key)
    +        sources = sorted(e.source for e in load_pool("openrouter").entries())
    +        assert sources == ["env:OPENROUTER_API_KEY", "manual"]
    +
    +    def test_manual_delete_adds_no_suppression(self):
    +        """Manual entries aren't re-seeded — CLI parity: no suppression marker."""
    +        from hermes_cli.auth import _load_auth_store
    +
    +        self.client.post(
    +            "/api/credentials/pool",
    +            json={"provider": "openrouter", "api_key": "[redacted:secret-assignment]" + "m" * 20},
    +        )
    +        assert self.client.delete("/api/credentials/pool/openrouter/1").status_code == 200
    +        suppressed = _load_auth_store().get("suppressed_sources", {})
    +        assert not suppressed.get("openrouter")
    +
    +        # Immediate re-add works.
    +        r = self.client.post(
    +            "/api/credentials/pool",
    +            json={"provider": "openrouter", "api_key": "[redacted:secret-assignment]" + "n" * 20},
    +        )
    +        assert r.status_code == 200 and r.json()["count"] == 1
    +
    +    def test_delete_does_not_clobber_other_providers(self):
    +        """Deleting one provider's env entry leaves other providers' rows alone."""
    +        from agent.credential_pool import load_pool
    +        from hermes_cli.auth import _load_auth_store, read_credential_pool
    +        from hermes_cli.config import save_env_value
    +
    +        self.client.post(
    +            "/api/credentials/pool",
    +            json={"provider": "anthropic", "api_key": "[redacted:secret-assignment]" + "k" * 20},
    +        )
    +        save_env_value("OPENROUTER_API_KEY", "sk-or-" + "q" * 20)
    +        load_pool("openrouter")
    +
    +        assert self.client.delete("/api/credentials/pool/openrouter/1").status_code == 200
    +
    +        assert len(read_credential_pool("anthropic")) == 1
    +        suppressed = _load_auth_store().get("suppressed_sources", {})
    +        assert list(suppressed.keys()) == ["openrouter"]
    +
     
     class TestMemoryEndpoints:
         @pytest.fixture(autouse=True)