mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 22:35:08 +00:00
fix(starlark): the store authenticated with a key nothing writes (#541)
#535 restored the thirteen routes, so the store stopped answering 404 -- and still would not load. Confirmed against a running device before anything was changed: /repository/browse answers 200 with 1000 apps in 27s, so the routes are fine. Two things underneath them are not. **The store never used the token the user configured.** The three repository routes read `github_token` off config.json. Nothing writes that key -- it is not in config.template.json, no setting offers it, and it appears nowhere else in the codebase. The configured token goes to config_secrets.json as `github.api_token`, which PluginStoreManager loads and every other GitHub caller uses. So the store could never be authenticated: 60 requests/hour, on the same per-IP budget 48 installed plugins spend on update checks, while the 5000 the user had already configured sat unused. On the device, /plugins/store/github-status reported authenticated with a limit of 5000 at the same moment /starlark/repository/browse reported 60, with 18 left. The store going blank was that 60 running out. **Every failure looked identical.** list_all_apps_cached turned any listing failure -- rate limit, DNS, timeout, non-200 -- into an empty app list, and the route sent that out as `status: success`, so a rate limit and an empty repository drew the same blank grid with no error anywhere. It now returns the reason, the route answers 502 with it, and a failure is no longer cached as an empty repository for two hours. The guard for a bad response was itself a crash: _make_request catches `(json.JSONDecodeError, ValueError)` but `json` was never imported, so evaluating the tuple raises NameError and the guard written for exactly this case never ran. Reachable whenever something on the path answers with HTML -- a captive portal, a proxy page, a DNS-hijacking router. Seventeen handlers answered 5xx with no detail at all. test_no_api_v3_handler_discards_its_exception is meant to prevent that across api_v3, but it matched one exact message string, and all thirteen Starlark routes wrote their own wording. The guard now keys on the shape that matters: if it returns 5xx, it says why. The 15 pre-existing non-Starlark functions are listed as a set that may shrink, never grow. **The listing was capped at 1000 and did not say so.** The contents API truncates a directory silently; tronbyt/apps has 1075 app directories, so the store showed a truncated repository and looked complete doing it. Now listed via the git trees API, which reports `truncated`, with the contents API kept as a fallback. Not addressed: the 27-second cold load -- 1075 manifests fetched five at a time behind skeleton placeholders -- which is probably the largest part of what "does not load" feels like, and wants its own change. 25 new tests. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -116,7 +116,56 @@ class TestHandlersCarryDetail:
|
||||
|
||||
src = open("web_interface/blueprints/api_v3.py").read()
|
||||
tree = ast.parse(src)
|
||||
generic = "An error occurred; see logs for details"
|
||||
|
||||
# This used to match one exact message string, so a handler that wrote
|
||||
# its own wording was never checked. All thirteen Starlark routes did
|
||||
# -- "Failed to browse repository" and friends -- and every one of them
|
||||
# answered a 500 with no detail at all, which is how the app store
|
||||
# spent three releases failing for reasons nobody could read. The rule
|
||||
# is now the shape that matters: if it returns 5xx, it says why.
|
||||
PRE_EXISTING = {
|
||||
# Not part of this change. This set may shrink, never grow.
|
||||
'backup_delete', 'backup_export', 'backup_list', 'backup_preview',
|
||||
'backup_restore', 'backup_validate', 'checkout_branch',
|
||||
'execute_system_action', 'get_git_branches', 'get_git_info',
|
||||
'get_hardware_status', 'get_logs', 'get_system_status',
|
||||
'get_system_version', 'scan_wifi_networks',
|
||||
}
|
||||
|
||||
def enclosing_function(handler):
|
||||
"""Innermost function containing `handler`."""
|
||||
best = None
|
||||
for fn in [n for n in ast.walk(tree)
|
||||
if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))]:
|
||||
if any(h is handler for h in ast.walk(fn)):
|
||||
if best is None or fn.lineno > best.lineno:
|
||||
best = fn
|
||||
return best.name if best else '<module>'
|
||||
|
||||
def only_catches_importerror(handler):
|
||||
"""An `except ImportError` arm and nothing else.
|
||||
|
||||
A missing optional dependency is a configuration fact, not a
|
||||
crash: the module name is the whole diagnosis and it is already
|
||||
in the response, so a stack trace would be noise. Detail is still
|
||||
required -- only the traceback log is excused.
|
||||
"""
|
||||
t = handler.type
|
||||
names = ([t] if isinstance(t, ast.Name)
|
||||
else list(t.elts) if isinstance(t, ast.Tuple) else [])
|
||||
return bool(names) and all(
|
||||
isinstance(n, ast.Name) and n.id == 'ImportError' for n in names)
|
||||
|
||||
def returns_5xx(handler):
|
||||
for r in [n for n in ast.walk(handler) if isinstance(n, ast.Return)]:
|
||||
v = r.value
|
||||
if isinstance(v, ast.Tuple) and len(v.elts) == 2:
|
||||
code = v.elts[1]
|
||||
if (isinstance(code, ast.Constant)
|
||||
and isinstance(code.value, int)
|
||||
and 500 <= code.value < 600):
|
||||
return True
|
||||
return False
|
||||
|
||||
def logs_a_traceback(handler):
|
||||
"""An error/exception-level log call carrying exc_info."""
|
||||
@@ -161,11 +210,12 @@ class TestHandlersCarryDetail:
|
||||
|
||||
offenders = []
|
||||
for h in [n for n in ast.walk(tree) if isinstance(n, ast.ExceptHandler)]:
|
||||
seg = ast.get_source_segment(src, h) or ""
|
||||
if generic not in seg:
|
||||
if not returns_5xx(h):
|
||||
continue
|
||||
if enclosing_function(h) in PRE_EXISTING:
|
||||
continue
|
||||
missing = []
|
||||
if not logs_a_traceback(h):
|
||||
if not logs_a_traceback(h) and not only_catches_importerror(h):
|
||||
missing.append("error-level log with exc_info")
|
||||
if not returns_the_detail(h):
|
||||
missing.append("describe_exception(e) in the response")
|
||||
|
||||
Reference in New Issue
Block a user