[codex] surface refresh auth and /data permission failures#39
[codex] surface refresh auth and /data permission failures#39zachyzissou merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves operator and user-facing diagnostics around refresh authorization failures and /data permission issues, while making the WebUI reflect asynchronous refresh behavior more clearly via /status polling.
Changes:
- WebUI: poll
/statusafter triggering/refresh, parse API error details, and displayrefresh_access_modein quick stats. - API/ops: add
checks.data_dir_writable, includerefresh_access_modein/healthand/status, and addHEAD /healthfor probe compatibility. - Tests/docs: extend integration assertions for the new fields and HEAD endpoint; add troubleshooting guidance for unauthorized refresh and
/datapermission errors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
web/assets/app.js |
Adds refresh-status polling, better refresh error messaging, and displays refresh_access_mode in the UI. |
app/server.py |
Adds refresh_access_mode helper, checks.data_dir_writable, and HEAD /health. |
test_integration.py |
Adds assertions for refresh_access_mode, data_dir_writable, and HEAD /health. |
README.md |
Updates troubleshooting guidance for refresh auth and /data permission failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data_dir_exists = DATA_DIR.exists() | ||
| data_dir_writable = data_dir_exists and os.access(DATA_DIR, os.W_OK) | ||
|
|
||
| # Check basic functionality | ||
| health_status = { | ||
| "status": "healthy", | ||
| "timestamp": datetime.now(UTC).isoformat(), | ||
| "checks": { | ||
| "api": "ok", | ||
| "data_dir": "ok" if DATA_DIR.exists() else "error", | ||
| "data_dir": "ok" if data_dir_exists else "error", | ||
| "data_dir_writable": "ok" if data_dir_writable else "error", | ||
| "web_assets": "ok" if (WEB_DIR / "assets").exists() else "warning", |
There was a problem hiding this comment.
data_dir_writable uses os.access(DATA_DIR, os.W_OK) which isn’t sufficient for directories (you typically need execute permission as well to create files, and os.access can still be a false-positive depending on filesystem semantics). To avoid reporting writable when the app can’t actually write, check for both W_OK | X_OK at minimum, or perform a lightweight create/delete probe in DATA_DIR (and treat failures as not writable).
| const btn = event.currentTarget; | ||
| const originalText = btn.textContent; | ||
| const beforeStatus = await fetchStatusSnapshot(); | ||
| const previousLastUpdate = beforeStatus?.last_update ?? null; |
There was a problem hiding this comment.
The refresh click handler captures originalText = btn.textContent and later restores it via btn.innerHTML = originalText. This drops the original <span aria-hidden="true">↻</span> markup from index.html and can make the icon text announced by screen readers (accessibility regression). Store/restore btn.innerHTML (or rebuild the original DOM structure) instead of using textContent for the reset path.
| byId('refresh').addEventListener('click', async event => { | ||
| const btn = event.currentTarget; | ||
| const originalText = btn.textContent; | ||
| const beforeStatus = await fetchStatusSnapshot(); | ||
| const previousLastUpdate = beforeStatus?.last_update ?? null; | ||
|
|
There was a problem hiding this comment.
fetchStatusSnapshot() is awaited before the button is disabled/loading state is set. If /status is slow/hung, the user can click multiple times and trigger multiple concurrent refresh handlers before the UI locks, potentially causing multiple /refresh calls. Consider disabling the button / setting aria-busy before awaiting the status snapshot (or reusing the last loaded status).
| def _refresh_access_mode() -> str: | ||
| """Return configured refresh access mode for diagnostics/UI.""" | ||
| if ALLOW_ANONYMOUS_LOCAL_REFRESH: | ||
| return "lan_anonymous" | ||
| if APP_REFRESH_TOKEN: | ||
| return "token_required" | ||
| return "disabled" |
There was a problem hiding this comment.
_refresh_access_mode() can return an inaccurate mode when both ALLOW_ANONYMOUS_LOCAL_REFRESH and APP_REFRESH_TOKEN are set. In that configuration, /refresh is allowed from LAN without auth and allowed elsewhere with the token, but this function reports only lan_anonymous. Consider returning a distinct value for the combined case (or exposing both flags separately) so /status, /health, and the WebUI don’t mislead operators.
Summary
/refreshreturns 401/statusafter refresh trigger so the UI reflects async generation instead of appearing as a no-oprefresh_access_modein quick statschecks.data_dir_writablerefresh_access_modein/healthand/statusHEAD /healthendpoint for probe compatibilityrefresh_access_mode,data_dir_writable, andHEAD /health/datapermission-denied scenariosValidation
python3 -m black app/server.py test_integration.pypython3 -m py_compile app/server.py test_integration.pynpm run -s lint:jsnpm run -s lint:mdNotes