mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
feat(debug): drop dead confirm step from --nous upload (stateless NAS)
NAS PR #349 (merged) ships a stateless presigned-PUT endpoint: the only route is POST /api/diagnostics/upload-url, and the object's existence in S3 is the only state. There is no /api/diagnostics/confirm route — confirming live against the merged preview returns 404. The client's confirm_upload() therefore fired a guaranteed-404 request on every --nous upload (harmless, since errors were swallowed, but dead). Remove it and simplify share_to_nous() to the 2-step mint + PUT flow that matches the shipped contract. Drop the corresponding TestConfirmUpload class and confirm assertions; add a test that the share succeeds even when the response carries no id (we no longer depend on it). The separately-flagged cross-repo requirement from #349's review -- sizeBytes is now REQUIRED and signed into the presigned URL's ContentLength -- was already satisfied: share_to_nous() sends len(bundle) as sizeBytes and urllib sets a matching Content-Length on the PUT. Verified against the live merged preview (missing sizeBytes -> 400 invalid_body; present -> 503 dark). Tested: pytest tests/hermes_cli/test_diagnostics_upload.py tests/hermes_cli/test_debug.py -> 95 passed.
This commit is contained in:
Ben
Teknium
parent
51eeb70cb8
commit
89653db403
2 changed files with 22 additions and 81 deletions
|
|
@ -10,8 +10,12 @@ Google-OAuth-gated viewer.
|
|||
Flow:
|
||||
|
||||
1. POST {NAS_BASE}/api/diagnostics/upload-url → {uploadUrl, viewUrl, id, ...}
|
||||
(the request body carries ``sizeBytes``; NAS signs it into the presigned
|
||||
URL's ``ContentLength``, so the PUT must send exactly that many bytes)
|
||||
2. PUT <uploadUrl> (the gzipped bundle, Content-Type application/gzip)
|
||||
3. POST {NAS_BASE}/api/diagnostics/confirm (best-effort; failures swallowed)
|
||||
|
||||
NAS is stateless — the object's existence in S3 is the only state, so there is
|
||||
no confirm/callback step.
|
||||
|
||||
Uses stdlib ``urllib`` only, matching ``debug.py`` style — no third-party deps.
|
||||
"""
|
||||
|
|
@ -115,40 +119,15 @@ def put_bundle(
|
|||
raise RuntimeError(f"diagnostics bundle PUT failed: HTTP {status}")
|
||||
|
||||
|
||||
def confirm_upload(upload_id: str, size_bytes: int) -> None:
|
||||
"""Best-effort notify NAS that the upload completed.
|
||||
|
||||
Purely advisory — the object's existence in S3 is the source of truth
|
||||
(NAS is stateless per the design), so any failure here is swallowed and
|
||||
must NOT abort a share that already succeeded.
|
||||
"""
|
||||
try:
|
||||
payload = json.dumps(
|
||||
{"id": upload_id, "sizeBytes": int(size_bytes)}
|
||||
).encode("utf-8")
|
||||
req = urllib.request.Request(
|
||||
f"{NAS_BASE}/api/diagnostics/confirm",
|
||||
data=payload,
|
||||
method="POST",
|
||||
headers={
|
||||
"Content-Type": "application/json",
|
||||
"User-Agent": _USER_AGENT,
|
||||
},
|
||||
)
|
||||
with urllib.request.urlopen(req, timeout=_REQUEST_TIMEOUT):
|
||||
pass
|
||||
except Exception:
|
||||
# Confirm is advisory only — never let it surface to the caller.
|
||||
pass
|
||||
|
||||
|
||||
def share_to_nous(report_bundle: bytes) -> dict:
|
||||
"""Orchestrate the full Nous-S3 upload of a gzipped *report_bundle*.
|
||||
|
||||
Returns the dict from :func:`request_upload_url` (which carries
|
||||
``viewUrl`` / ``id`` / expiry metadata) so the caller can print the
|
||||
viewer link. Raises on any failure of the required steps (mint URL or
|
||||
PUT); the confirm step is best-effort and never raises.
|
||||
Two steps: mint a presigned PUT URL (sending the exact ``sizeBytes`` NAS
|
||||
signs into the URL's ``ContentLength``), then PUT the bundle. NAS is
|
||||
stateless — the object's existence in S3 is the only state, so there is no
|
||||
confirm/callback step. Returns the dict from :func:`request_upload_url`
|
||||
(which carries ``viewUrl`` / ``id`` / expiry metadata) so the caller can
|
||||
print the viewer link. Raises on any failure of either step.
|
||||
"""
|
||||
size_bytes = len(report_bundle)
|
||||
info = request_upload_url(
|
||||
|
|
@ -156,8 +135,4 @@ def share_to_nous(report_bundle: bytes) -> dict:
|
|||
)
|
||||
put_bundle(info["uploadUrl"], report_bundle, content_type="application/gzip")
|
||||
|
||||
upload_id = info.get("id")
|
||||
if upload_id:
|
||||
confirm_upload(upload_id, size_bytes)
|
||||
|
||||
return info
|
||||
|
|
|
|||
|
|
@ -176,43 +176,12 @@ class TestPutBundle:
|
|||
put_bundle("https://u", b"data")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# confirm_upload
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestConfirmUpload:
|
||||
def test_confirm_posts(self):
|
||||
from hermes_cli.diagnostics_upload import confirm_upload
|
||||
|
||||
resp = _resp(status=200, body=b"")
|
||||
with patch(
|
||||
"hermes_cli.diagnostics_upload.urllib.request.urlopen",
|
||||
return_value=resp,
|
||||
) as urlopen:
|
||||
confirm_upload("abc-123", 1024)
|
||||
req = urlopen.call_args[0][0]
|
||||
assert req.method == "POST"
|
||||
assert req.full_url.endswith("/api/diagnostics/confirm")
|
||||
sent = json.loads(req.data.decode())
|
||||
assert sent == {"id": "abc-123", "sizeBytes": 1024}
|
||||
|
||||
def test_confirm_failure_is_swallowed(self):
|
||||
from hermes_cli.diagnostics_upload import confirm_upload
|
||||
|
||||
with patch(
|
||||
"hermes_cli.diagnostics_upload.urllib.request.urlopen",
|
||||
side_effect=urllib.error.URLError("connection refused"),
|
||||
):
|
||||
# Must NOT raise.
|
||||
confirm_upload("abc-123", 1024)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# share_to_nous (orchestration)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestShareToNous:
|
||||
def test_orchestrates_request_put_confirm(self):
|
||||
def test_orchestrates_request_then_put(self):
|
||||
from hermes_cli import diagnostics_upload as mod
|
||||
|
||||
info = {
|
||||
|
|
@ -224,38 +193,35 @@ class TestShareToNous:
|
|||
blob = b"gzipped-bundle"
|
||||
|
||||
with patch.object(mod, "request_upload_url", return_value=info) as req, \
|
||||
patch.object(mod, "put_bundle") as put, \
|
||||
patch.object(mod, "confirm_upload") as confirm:
|
||||
patch.object(mod, "put_bundle") as put:
|
||||
result = mod.share_to_nous(blob)
|
||||
|
||||
assert result == info
|
||||
req.assert_called_once()
|
||||
# request was told the real byte size
|
||||
# request was told the real byte size (NAS signs it into ContentLength)
|
||||
assert req.call_args.kwargs["size_bytes"] == len(blob)
|
||||
# PUT got the signed URL + the exact blob
|
||||
put.assert_called_once_with(
|
||||
info["uploadUrl"], blob, content_type="application/gzip"
|
||||
)
|
||||
confirm.assert_called_once_with("id-9", len(blob))
|
||||
|
||||
def test_put_failure_propagates(self):
|
||||
from hermes_cli import diagnostics_upload as mod
|
||||
|
||||
info = {"id": "id-9", "uploadUrl": "https://u", "viewUrl": "v"}
|
||||
with patch.object(mod, "request_upload_url", return_value=info), \
|
||||
patch.object(mod, "put_bundle", side_effect=RuntimeError("PUT failed")), \
|
||||
patch.object(mod, "confirm_upload") as confirm:
|
||||
patch.object(mod, "put_bundle", side_effect=RuntimeError("PUT failed")):
|
||||
with pytest.raises(RuntimeError):
|
||||
mod.share_to_nous(b"data")
|
||||
# confirm never runs if PUT failed
|
||||
confirm.assert_not_called()
|
||||
|
||||
def test_confirm_skipped_without_id(self):
|
||||
def test_share_succeeds_without_id_in_response(self):
|
||||
from hermes_cli import diagnostics_upload as mod
|
||||
|
||||
# NAS is stateless and there is no confirm step, so the share must
|
||||
# succeed regardless of whether the response carries an ``id``.
|
||||
info = {"uploadUrl": "https://u", "viewUrl": "v"} # no id
|
||||
with patch.object(mod, "request_upload_url", return_value=info), \
|
||||
patch.object(mod, "put_bundle"), \
|
||||
patch.object(mod, "confirm_upload") as confirm:
|
||||
mod.share_to_nous(b"data")
|
||||
confirm.assert_not_called()
|
||||
patch.object(mod, "put_bundle") as put:
|
||||
result = mod.share_to_nous(b"data")
|
||||
assert result == info
|
||||
put.assert_called_once()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue