fix: load Terminal assets under bearer auth - #8
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f5d3004bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 5f5d3004bd6d · formal
Overall risk is low: the PR makes page rendering config-driven (view.py's render_page + api.py serving a per-config page), and the only surviving finding is a confirmed stale-docstring nit — nothing blocking or major. Fix the view.py module docstring first; it still claims "api.py returns it on GET /view", which this PR made false. The panel had no disagreement; verification changed one thing — the head-SHA pin 404'd (orphaned ref), so it re-read the base branch plus the PR diff and confirmed the docstring sits in no hunk and is stale byte-for-byte (confirmed). Coverage gap: api.py is touched by the diff (import swap, render call, return) yet has zero findings — that path went under-read, and no tool-sourced (protopatch) findings were in the panel input to corroborate it.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | view.py:18 |
The module docstring is stale after this PR: it says api.py returns PAGE on GET /view, but api.py now renders the page from config via render_page() and no lon… | confirmed |
findings JSON (machine-readable)
[
{
"file": "view.py",
"line": 18,
"severity": "nit",
"category": "conventions",
"claim": "The module docstring is stale after this PR: it says api.py returns PAGE on GET /view, but api.py now renders the page from config via render_page() and no longer imports PAGE at all.",
"evidence": "Docstring, identical at PR head: \"No build step \u2014 vanilla JS; xterm.js + addons are VENDORED and served by this plugin\n(offline). ``PAGE`` is the HTML; api.py returns it on GET /view.\" \u2014 while the diff changes api.py to \"from .view import render_page\" and \"return HTMLResponse(page)\".",
"verdict": "confirmed",
"note": "Re-read at head: view.py diff touches only the PAGE_TEMPLATE rename, the __FONT_SIZE__/__SCROLLBACK__ line, and the tail; the docstring is in no hunk and reads verbatim '``PAGE`` is the HTML; api.py returns it on GET /view.' (joined from two physical lines). api.py diff is exactly -from .view import PAGE / +from .view import render_page and -return HTMLResponse(PAGE) / +return HTMLResponse(page) with page = render_page(font_size=..., scrollback=...) \u2014 so api.py no longer imports PAGE and returns a config-rendered page. Docstring is stale; nit severity appropriate."
}
]
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 30beb27e9db3 · formal
Low-risk, well-scoped config-render PR (api.py _int_setting + render_page wiring, view.py's new render_page, manifest gains only public_paths). Both verified findings survive as nits — nothing gates the merge; fix-first is a test for _int_setting's fallback branch, the only untested path. Verification changed nothing substantively: the verifier confirmed both nits on byte-grounded diff evidence and refuted nothing; there was no panel disagreement. Gaps: PR-head file reads 404'd (pinned SHA 30beb27 orphaned), so plus-side quotes rest on the diff plus a base-branch manifest read; the round-1 nit (stale module docstring, view.py:18) was not re-verified this round and remains open as a nit; no structural pass was skipped (the structural verify completed).
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | api.py:80 |
The new _int_setting fallback branch (invalid or non-integer config values falling back to the default) is the validation half of the PR's config-render chan… |
confirmed |
| ⚪ | nit | api.py:83 |
The terminal defaults 13 and 5000 are now duplicated in three places — the render_page signature, these _int_setting calls, and the manifest config block — so … | confirmed |
findings JSON (machine-readable)
[
{
"file": "api.py",
"line": 80,
"severity": "nit",
"category": "tests",
"claim": "The new `_int_setting` fallback branch (invalid or non-integer config values falling back to the default) is the validation half of the PR's config-render change but is entirely untested \u2014 only the happy path (font_size 17 / scrollback 1234) has coverage, so a regression that removes the try/except would pass CI.",
"evidence": "except (TypeError, ValueError):\n return default\n\n page = render_page(font_size=_int_setting(\"font_size\", 13), scrollback=_int_setting(\"scrollback\", 5000))",
"verdict": "confirmed",
"note": "Evidence is verbatim on the + side of the PR diff (api.py hunk); the diff adds exactly one test \u2014 test_view_applies_configured_font_size_and_scrollback with valid ints 17/1234 \u2014 so the except branch is never hit and removing try/except would pass CI. PR-head file reads 404'd (pinned SHA orphaned) but the diff itself grounds the claim."
},
{
"file": "api.py",
"line": 83,
"severity": "nit",
"category": "conventions",
"claim": "The terminal defaults 13 and 5000 are now duplicated in three places \u2014 the render_page signature, these _int_setting calls, and the manifest config block \u2014 so a default change in one source silently diverges from the others with no test to catch it.",
"evidence": "page = render_page(font_size=_int_setting(\"font_size\", 13), scrollback=_int_setting(\"scrollback\", 5000))",
"verdict": "confirmed",
"note": "All three locations verified: render_page(*, font_size: int = 13, scrollback: int = 5000) in the view.py diff; the _int_setting call verbatim in the api.py diff; and `scrollback: 5000` / `font_size: 13` read directly from the manifest config block (unchanged by this PR \u2014 diff only adds public_paths)."
}
]
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head fc3770251027 · formal
PR #8 adds config-driven terminal rendering to view.py (render_page backed by the new DEFAULT_FONT_SIZE/DEFAULT_SCROLLBACK constants) and the panel-requested fallback test in tests/test_api.py; overall risk is low and the panel had no disagreement. Fix first: the one surviving, verifier-confirmed nit — the fallback test hardcodes 13/5000 as string literals instead of asserting against the constants, so a legitimate default change fails it spuriously; assert against DEFAULT_FONT_SIZE/DEFAULT_SCROLLBACK. Verification confirmed the nit from the diff (the added lines match verbatim) and changed nothing else. Gaps: PR-head still 404s, so the verdict rests on diff + base reads only, and view.py — the production change — carries zero findings (under-read; never read at head). Prior rounds raised only nits (stale view.py docstring, default duplication), so no blocker/major dispositions apply; the duplication nit is effectively superseded by the constants this PR introduces.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | tests/test_api.py:66 |
The new fallback test re-hardcodes the terminal defaults 13/5000 as literals instead of comparing against the DEFAULT_FONT_SIZE/DEFAULT_SCROLLBACK constants th… | confirmed |
findings JSON (machine-readable)
[
{
"file": "tests/test_api.py",
"line": 66,
"severity": "nit",
"category": "conventions",
"claim": "The new fallback test re-hardcodes the terminal defaults 13/5000 as literals instead of comparing against the DEFAULT_FONT_SIZE/DEFAULT_SCROLLBACK constants this PR introduced, so a legitimate default change now fails the fallback test spuriously \u2014 the exact drift the constants were added to eliminate.",
"evidence": "assert \"fontSize: 13\" in body\n assert \"scrollback: 5000\" in body",
"verdict": "confirmed",
"note": "Diff-verified: the added fallback test asserts the literals 'fontSize: 13'/'scrollback: 5000' (verbatim in the diff's added lines) while the same PR adds DEFAULT_FONT_SIZE=13/DEFAULT_SCROLLBACK=5000 in view.py and render_page() renders from them \u2014 a default change would fail this test spuriously. Stands on its own evidence (implementation defect in the requested fallback coverage, not a re-request for tests). Line 66 is a stale anchor; quote located by code."
}
]
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 0921b8ce0ffd · formal
Overall risk is low-to-moderate: the change is small and the panel surfaces a single confirmed minor — _int_setting's except (TypeError, ValueError) misses OverflowError, so a YAML config value that resolves to a float infinity (.inf, 1e999) crashes build_router at plugin load instead of falling back, which is the exact invalid-config containment the try/except was added for. Fix that first by widening the except tuple (or catching ArithmeticError) at api.py:79. Verification changed nothing substantively: the verifier re-anchored to the live head because pinned SHA 0921b8c was orphaned by a force-push, confirmed the quote and substance byte-for-byte, and kept the finding as-is — no refutations, no uncertain items; the earlier-round nits (stale view.py docstring, triplicated defaults, hardcoded fallback-test literals) are not re-litigated and sit at nit. Coverage gaps: no tool-sourced (protopatch/structural) findings appear this round, so the structural pass either skipped or found nothing on api.py — treat that file as under-read, and tests/test_api.py plus view.py likewise carry no findings this round.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | api.py:79 |
_int_setting does not catch OverflowError, so a config value the YAML loader resolves to a float infinity (e.g. font_size: .inf or 1e999) makes int() rai… |
confirmed |
findings JSON (machine-readable)
[
{
"file": "api.py",
"line": 79,
"severity": "minor",
"category": "correctness",
"claim": "`_int_setting` does not catch OverflowError, so a config value the YAML loader resolves to a float infinity (e.g. `font_size: .inf` or `1e999`) makes int() raise and crashes build_router at plugin load with an unhandled exception instead of falling back to the default \u2014 the exact invalid-config containment this try/except was added to provide (the new fallback test covers only ValueError and TypeError paths).",
"evidence": "try:\n return int((cfg or {}).get(name, default))\n except (TypeError, ValueError):\n return default",
"verdict": "confirmed",
"note": "Re-read api.py at PR head (fork branch fix/issue-7-bearer-assets; pinned SHA 0921b8ce was orphaned by a force-push, re-anchored to live head \u2014 diff and raw file agree): _int_setting catches only (TypeError, ValueError); int(float('inf')) raises OverflowError (verified Python behavior), a sibling of ArithmeticError not covered by that tuple, and the _int_setting calls run at build_router top level, so a YAML .inf/1e999 config (PyYAML resolves both to inf) propagates unhandled at plugin load; the fallback test exercises only 'huge'/None (ValueError/TypeError). Claim is technically accurate; severity minor is fair given loader-dialect reachability."
}
]1 finding(s) downgraded to uncertain: the code they quote as evidence does not appear in the file at the reviewed head, nor in this PR's patch for it. A finding that cannot be grounded does not gate a merge (issue #25) — it still stands for a human to judge.
api.py(minor) — quoted evidence not found at this head:font_size: .inf
|
Addressed the remaining review feedback at
Repository-local verification: Prepared by Hermes with AI assistance. |
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 16350737e0b9 · formal
This round produced zero findings, but not because the PR is clean: the verifier reports the diff it fetched for PR #8 was pulled from the wrong repo context and doesn't match the terminal-plugin's api.py/view.py that every prior round reviewed. No finder actually read this PR's real diff this pass, so there is nothing to adjudicate and nothing new to disposition — treat this as an inconclusive/skipped review, not a clean PASS.
Fix first: re-run the review with the correct repo/diff context before relying on any "no findings" result here. Nothing else can be prioritized until that's fixed.
Nothing to disagree on this round — no findings were produced by anyone to compare.
Prior history had no open blocker/major (all prior requests were nit or minor severity), so no disposition table is required. The one open item worth tracking forward as an existing PR thread — round 4's minor on api.py:79 (_int_setting not catching OverflowError on float-infinity config values) — could not be re-verified this round since the diff was unreadable; it stays wherever that thread currently sits.
Coverage gap: the structural/finder pass was effectively skipped this round due to the diff-fetch mismatch — the historically load-bearing files api.py and view.py went completely unread this pass.
No findings — the review came back clean.
findings JSON (machine-readable)
[]
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 16350737e0b9 · formal
Overall risk this round is high: the sole surviving finding is a genuine blocker in graph/agent.py — a bare import tracing was added inside task() where no top-level tracing module exists (the real module is observability.tracing, as every other caller correctly imports it via from observability import tracing). Because the import sits before the function's own try/except, it raises ModuleNotFoundError unconditionally, breaking every call to task(), not just the new emit_skill=True path. Fix first: change the import to from observability import tracing, matching the existing convention in graph/middleware/trace_context.py.
No panel disagreement to report — this finding was independently confirmed by direct diff inspection and by reading observability/tracing.py plus an existing correct caller; the verifier found no basis to soften or refute it, so it stays a blocker as-is. Verification added cross-file corroboration (another caller using the correct import path) that strengthens the original claim.
Prior rounds (1–4) surfaced only nits and one minor (missing OverflowError handling in _int_setting), none rising to blocker/major, so there are no prior blocker/major dispositions to carry forward this round — the dispositions table below is empty by design, not an omission.
Coverage gap: only one finding reached this synthesis pass (from the structural verifier), covering graph/agent.py. No structural-pass skip was observed — the workflow that produced this finding is exactly the structural verify pass — but the rest of the diff (if any files beyond graph/agent.py changed this round) has no finder output recorded here, so its coverage status is unknown.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🔴 | blocker | graph/agent.py |
task() in graph/agent.py adds an unconditional bare import tracing, which resolves to no module (the real module is observability.tracing), so every call t… |
confirmed |
findings JSON (machine-readable)
[
{
"file": "graph/agent.py",
"line": 0,
"severity": "blocker",
"category": "correctness",
"claim": "task() in graph/agent.py adds an unconditional bare `import tracing`, which resolves to no module (the real module is `observability.tracing`), so every call to task() raises ModuleNotFoundError before the function's own try/except can catch it.",
"evidence": "+ import tracing",
"verdict": "confirmed",
"note": "All parts of this finding are confirmed by direct diff inspection and by reading the actual `observability/tracing.py` module and an existing correct caller \u2014 keep as a blocker, no qualification needed."
}
]1 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):
graph/agent.py(blocker) — task() in graph/agent.py adds an unconditional bareimport tracing, which resolves to no module (the real module isobservability.tracing), so every call to
|
Current head Prepared by Hermes with AI assistance. |
1 participant
Problem
Terminal's public view loads without a bearer, but its browser-requested vendored xterm assets are not included in the plugin's public-path contract. On a bearer-gated protoAgent host, the page returns 200 while its scripts and stylesheet return 401, leaving the Terminal panel unusable.
The view also hardcodes
fontSize: 13andscrollback: 5000, so the corresponding manifest settings have no effect.What changed
/plugins/terminal/static/as a public plugin path. The asset handler still serves only its exact filename allowlist; unknown names remain 404.terminal.font_sizeandterminal.scrollbackvalues into the xterm constructor.mainuntil maintainers publish a release containing this fix, then direct users to pin that exact release; also explain fetch-only CLI behavior versus manifest-default enablement.Proof
pytest -q: 23 passed at current head16350737e0b9b79d2827c9324988a334d03373de.ruff check .: passed.ruff format --check .: passed./api/plugins/terminal/*remains gated.Addresses the plugin-local portions of #7. The generic protoAgent CLI
NOT enabledwording is a separate core reporting mismatch and is not changed here.Upstream suitability: this restores the plugin's documented behavior on any bearer-gated protoAgent host without adding installation-specific assumptions.