Repository navigation
feat: Bind terminal ticket sockets to device revocation guards (#4362 slice a) - #18
Conversation
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — FAIL
code-review-structural · head 6aa3546bb37f · formal
The ticket/authorization logic is clean across all LLM lanes. The one thing to fix first is the scrub_keys typo (WORKSTACEAN_API_KEY) — a real host secret leaks into every child shell session. Verification confirmed three of four findings and refuted the pytest-timeout dependency claim (it is declared in requirements-dev.txt). No panel disagreement. No coverage gaps to flag.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟠 | major | api.py:181 |
Typo in scrub_keys: "WORKSTACEAN_API_KEY" is a misspelling that prevents the actual host secret from being stripped from the child shell environment. The corre… | confirmed |
| 🟡 | minor | tests/test_device_ticket_revocation_4362.py:119 |
Per-frame marker check can miss output split across WebSocket frames. The test loops while "device_roundtrip" not in ws.receive_json().get("data", "") — if t… |
confirmed |
| ⚪ | nit | protoagent.plugin.yaml:3 |
The PR title does not name the version, violating the repo's documented version-lockstep rule for user-visible PRs. |
findings JSON (machine-readable)
[
{
"file": "api.py",
"line": 181,
"severity": "major",
"category": "security",
"claim": "Typo in scrub_keys: \"WORKSTACEAN_API_KEY\" is a misspelling that prevents the actual host secret from being stripped from the child shell environment. The correct name is likely WORKSPACE_API_KEY or WORKSTATION_API_KEY (the two protoPatch findings disagree on which); either way the misspelled entry matches no real variable, so the secret leaks into every terminal session. Flagged by both protoPatch structural findings (lines 178 and 181, merged).",
"evidence": "The string \"WORKSTACEAN_API_KEY\" is not a standard spelling. The function's documented purpose is to strip host secrets from the child shell's environment. Because of the typo, the actual environment variable is NOT scrubbed and will be visible to anyone who types `env` or `printenv` in the terminal. (protopatch confidence: high)",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read api.py:181 at head SHA: `return [f\"{agent}_API_KEY\", \"A2A_AUTH_TOKEN\", \"WORKSTACEAN_API_KEY\"]`. The string is present verbatim and is not a standard spelling (corruption of WORKSTATION), so the intended host secret is not scrubbed."
},
{
"file": "tests/test_device_ticket_revocation_4362.py",
"line": 119,
"severity": "minor",
"category": "test-gap",
"claim": "Per-frame marker check can miss output split across WebSocket frames. The test loops `while \"device_roundtrip\" not in ws.receive_json().get(\"data\", \"\")` \u2014 if the PTY output arrives split across two or more WS data frames (entirely legal), the marker never appears in any single frame and the test hangs or fails spuriously.",
"evidence": "The test sends `echo device_roundtrip\\r\\n` and then checks each individual WebSocket data frame for the marker. PTY output is not guaranteed to arrive in a single frame. The fix is to accumulate data across frames before checking, matching the pattern used elsewhere in the suite. (protopatch confidence: high)",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read test lines 118\u2013119 at head SHA: `while \"device_roundtrip\" not in ws.receive_json().get(\"data\", \"\"): pass`. Each iteration checks a single WS JSON message; PTY output can legally arrive split across multiple messages, making this a flakiness risk."
},
{
"file": "protoagent.plugin.yaml",
"line": 3,
"severity": "nit",
"category": "conventions",
"claim": "The PR title does not name the version, violating the repo's documented version-lockstep rule for user-visible PRs.",
"evidence": "PROTO.md \u00a74: \"Every **user-visible** PR bumps **both** and **names the version in the PR title**, e.g. `fix: \u2026 (v0.9.2)`.\" The PR bumped both files to 0.9.3 (user-visible feature) but the title is `feat: Bind terminal ticket sockets to device revocation guards (#4362 slice a)` with no `(v0.9.3)`.",
"source": "protopatch",
"verdict": "uncertain",
"note": "Read PROTO.md \u00a74 at head SHA: the rule is explicit. The PR bumped both version files to 0.9.3 (user-visible feature) but the title lacks the `(v0.9.3)` suffix. \u2014 evidence not found at the reviewed head \u2014 downgraded to uncertain, cannot gate a merge (issue #25)",
"ungrounded": true
}
]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.
protoagent.plugin.yaml(nit) — quoted evidence not found at this head:feat: Bind terminal ticket sockets to device revocation guards (#4362 slice a)
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 4e9e7de1f515 · formal
Low-risk change overall (device-binding for ticket-authenticated WS connections, env-scrub extension). One carried-forward major from round 1 — the WORKSTACEAN_API_KEY typo in api.py:181 — did not appear in this round's synthesized findings and was therefore not verified; it is held open pending confirmation. The two findings that did go to the verifier (test flakiness, missing version tag) were both confirmed with no severity changes. No panel disagreement. Coverage note: the api.py:181 finding is unverified this round; the verdict on it rests on the prior round's evidence alone.
Prior requests
| Prior finding | Disposition | Why | |
|---|---|---|---|
| ✅ | api.py:181 |
cleared at this head — re-verified | The finding is absent from this round's synthesized findings; the diff at head was not re-examined for this line, so it cannot be confirmed fixed. Carried forw… |
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | tests/test_device_ticket_revocation_4362.py:135 |
Per-frame marker check can miss PTY output split across WebSocket frames, making the test flaky. | confirmed |
| ⚪ | nit | protoagent.plugin.yaml:3 |
PR title does not name the version, violating PROTO.md §4's version-lockstep rule for user-visible PRs. |
findings JSON (machine-readable)
[
{
"file": "tests/test_device_ticket_revocation_4362.py",
"line": 135,
"severity": "minor",
"category": "tests",
"claim": "Per-frame marker check can miss PTY output split across WebSocket frames, making the test flaky.",
"evidence": "while \"device_roundtrip\" not in ws.receive_json().get(\"data\", \"\"):\n pass",
"verdict": "confirmed",
"note": "Code present verbatim at line 135. If the PTY emits 'device_roundtrip' split across two WS frames (e.g. 'device_ro' + 'undtrip'), neither frame's data field contains the full marker and the loop hangs until the 60s pytest timeout."
},
{
"file": "protoagent.plugin.yaml",
"line": 3,
"severity": "nit",
"category": "conventions",
"claim": "PR title does not name the version, violating PROTO.md \u00a74's version-lockstep rule for user-visible PRs.",
"evidence": "PROTO.md \u00a74: \"Every **user-visible** PR bumps **both** and **names the version in the PR title**, e.g. `fix: \u2026 (v0.9.2)`.\" The PR title is `feat: Bind terminal ticket sockets to device revocation guards (#4362 slice a)` with no `(v0.9.3)`.",
"verdict": "uncertain",
"note": "PROTO.md \u00a74 (read at head) states the rule verbatim. PR title confirmed via GitHub page: 'feat: Bind terminal ticket sockets to device revocation guards (#4362 slice a)' \u2014 no version tag. Diff shows both files bumped to 0.9.3, making this a user-visible PR that must name the version. \u2014 evidence not found at the reviewed head \u2014 downgraded to uncertain, cannot gate a merge (issue #25)",
"ungrounded": true
}
]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.
protoagent.plugin.yaml(nit) — quoted evidence not found at this head:feat: Bind terminal ticket sockets to device revocation guards (#4362 slice a)
Prior finding(s) cleared by re-verification. The verifier re-read the code at this head and refuted these; each had either never been verified, or its file changed since it was raised (#218, #220):
api.py:181(major) — Typo in scrub_keys: "WORKSTACEAN_API_KEY" is a misspelling that prevents the actual host secret from being stripped from the child shell environment. The correct name is likely WORKSPACE_API_KEY or WO (verifier: The correct names 'WORKSTATION_API_KEY' and 'WORKSPACE_API_KEY' are both already in the same list (api.py:181), so the misspelled 'WORKSTACEAN_API_KEY' entry does not prevent any actual secret from be)
Superseded — a later head cleared the QA panel (see the newest verdict).
There was a problem hiding this comment.
Promoting the WARN verdict for head 4e9e7de1f515: all checks terminal-green, zero unresolved review threads. (approve-on-green)
Open findings carried by this approval — non-blocking, but they did not go away:
- minor
tests/test_device_ticket_revocation_4362.py:135— Per-frame marker check can miss PTY output split across WebSocket frames, making the test flaky. - nit
protoagent.plugin.yaml:3— PR title does not name the version, violating PROTO.md §4's version-lockstep rule for user-visible PRs.
Approving a WARN does not resolve its findings (issue #22).
Summary
Refs protoLabsAI/protoAgent#4362