Repository navigation
plugin: skip JSON lines that are not objects in the agent-output readers - #183
Rahul (rahul05ranjan) wants to merge 2 commits into
Conversation
|
Rahul (@rahul05ranjan) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
The readers of agent CLI output streams (probe.run_session, probe.inspect_session, sidecall.codex_call, sidecall.copilot_call, toolcheck._init_tools) parsed each line with json.loads and called .get on the result, so a line that parses but is not an object raised AttributeError and killed the whole probe, side call or tool check. inspect_session had no ValueError catch either, so one unparseable hook.end line crashed it too. microsoft#150 added the isinstance check to the trace readers; these five plugin-side readers of external process output are the same class. Add the guard everywhere, with regression tests. Fixes microsoft#182 Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com>
468b1b9 to
859bd01
Compare
Muhammad Abdullah Butt (1AbdullahButt)
left a comment
There was a problem hiding this comment.
I compared each changed loop with main. Four of the five changes are the straightforward isinstance guard, but the inspect_session rewrite changes what it reads.
tokeneconomizer/plugin/probe.py:155 drops the .get("output") step. On main it is:
out = (json.loads(line).get("data") or {}).get("output") or {}and the PR has:
out = (ev.get("data") or {}) if isinstance(ev, dict) else {}The next line still looks up out.get("additionalContext"), so it now reads data.additionalContext rather than data.output.additionalContext. A real Copilot hook.end / sessionStart line would then always report injected: False. The new test can't catch this, because its only well-formed hook line is a bare string and it asserts injected is False. Suggested:
data = ev.get("data") if isinstance(ev, dict) else None
out = (data.get("output") if isinstance(data, dict) else None) or {}
injected = injected or "fails here" in ((out.get("additionalContext") if isinstance(out, dict) else "") or "")A test with a valid {"type": "hook.end", "data": {"hookType": "sessionStart", "output": {"additionalContext": "... fails here ..."}}} line asserting injected is True would pin it.
The same crash is still one level down. The guard covers the top-level event, but the nested values are still trusted:
sidecall.py:317:item = ev.get("item") or {}, thenitem.get("type")raises on{"type": "item.completed", "item": "x"}.sidecall.py:731andprobe.py:136:(ev.get("data") or {}).get("content")raises on"data": [..].
These are less likely than a stray null line. If the aim of #182 is that one bad line never loses the call, an isinstance(..., dict) check on item and data (or a small _obj(v) helper that returns v if isinstance(v, dict) else {}) would cover them too.
Reviewed with help from Claude Code.
The non-object guard dropped .get('output'), so inspect_session always reported injected: False. Adds a test with a real hook.end line; both test logs end with session.shutdown so settle() does not wait 15 s.
Fixes #182.
Five readers of agent CLI output parsed each line with
json.loadsand then called.geton the result, so one line that parses but is not an object (null,42,"text",[1, 2]) raisedAttributeErrorand lost the whole probe / side call / tool check:probe.run_session— Copilot-p --output-format jsonstdoutprobe.inspect_session— Copilot'sevents.jsonl(also had noexcept ValueError, so a corrupt line carrying the"hook.end"prefilter crashed before.get)sidecall.codex_call—codex exec --jsonstdoutsidecall.copilot_call— Copilot-pstdouttoolcheck._init_tools—claude -p --output-format stream-jsonstdout#150 added the
isinstance(ev, dict)check to the trace readers; these plugin-side readers of external process output are the same class and were not in that sweep.Verification
AttributeErrorin all three, plus theValueErrorpath ininspect_session) and pass with the fix:test_the_side_calls_skip_json_lines_that_are_not_objects— junk lines (null,[1,2],"text",42) ahead of the realitem.completed/assistant.message+resultevents; codex and copilot side calls andprobe.run_sessionstill read the reply, session id and answer.test_inspect_session_skips_hook_lines_that_do_not_parse— an unparseable{broken ...hook.end line and a parses-but-not-an-object"hook.end sessionStart"string; the inspection completes and reportsinjected: False.test_the_init_probe_skips_json_lines_that_are_not_objects(tests/test_toolcheck_measure.py) — the realPopenprobe prints four junk lines before thesystem/initevent; tools and version still returned.python3 -m pytest tests/ -q) green with the fix; standard library only.