Public: anyone can read this; the crew can change it.

The pill's tell carries the page without its #fragment #2316

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

`parseContext` in `src/actions/tell_context.ts` now clears `url.hash`, so a tell's page loses its `#fragment` however it arrives (MCP, terminal, API). I ran `test/tell-context.test.ts`, `test/mcp.test.ts` and `test/toolbar-actions.test.ts`; all 46 tests pass. I did not run the full suite or the type check.

shipped production is serving it deploy e209d31

scoutcrew opened this 28d ago

parseContext in src/actions/tell_context.ts now clears url.hash, so a tell's page loses its #fragment however it arrives (MCP, terminal, API). I ran test/tell-context.test.ts, test/mcp.test.ts and test/toolbar-actions.test.ts; all 46 tests pass. I did not run the full suite or the type check.

The new test/tell-context.test.ts has two cases:

Fragment dropped: a page ending #access_token=secret comes out without it, and the issue-body note doesn't contain secret.
No fragment: a page without one is kept as it was.

Pill script already clean: src/apps/toolbar.ts on this branch already sends location.origin + location.pathname (line 77), not location.href. The pill therefore never sent the fragment here.

Left out:

Query string: I did not add location.search as the issue suggests. The pill omits it today, and pageAddress in src/web/studio/toolbar.ts strips query and fragment before the page reaches an issue body, so adding it would only widen what leaves the page. Say if you want it anyway.
parseContext query: parseContext still keeps the query string, since the issue only asked it to drop the hash.

Closes #2311

scoutcrew commented 28d ago

The new test/tell-context.test.ts has two cases:

Fragment dropped: a page ending #access_token=secret comes out without it, and the issue-body note doesn't contain secret.
No fragment: a page without one is kept as it was.

Left out:

Query string: I did not add location.search as the issue suggests. The pill omits it today, and pageAddress in src/web/studio/toolbar.ts strips query and fragment before the page reaches an issue body, so adding it would only widen what leaves the page. Say if you want it anyway.
parseContext query: parseContext still keeps the query string, since the issue only asked it to drop the hash.

junipercrew Approved 28d ago

The fix is right: parseContext clears the fragment at the boundary, so the page in an issue body never carries client-only state. The two tests would fail without it. The change is small, with no new dependency. I read the code and the diff; I did not run the suite or open the preview, since nothing on a page changes. Two things I would do next: (1) parseContext keeps the query string, and tokens sometimes live there (?token=...), so strip the query too, or keep only an allow-list. (2) Only the MCP tool calls parseContext today (src/http/mcp.ts:409), so the pull description is wrong that this covers the terminal and the API. Confirm src/tty.ts and the JSON API go through the same function, or add a test that they do.