chore: maintain security-sensitive UI surfaces - #39350
Conversation
- Add DOMPurify overrides and dependency hygiene updates across root, TUI, desktop, and website packages - Add launchd file descriptor limits for the gateway service - Add Company Brain trace parsing helpers for future desktop answer gating - Fix TypeScript compatibility issues in desktop, TUI, and website code
…ntenance # Conflicts: # apps/desktop/package-lock.json # hermes_cli/gateway.py # package-lock.json # ui-tui/package-lock.json
|
CI is currently fork-gated ( Local verification completed on the pushed head
Maintainers: please approve the fork workflows when convenient. Maintainer edits are enabled. |
austinpickett
left a comment
There was a problem hiding this comment.
This PR bundles four unrelated workstreams under a single "chore: maintain security-sensitive UI surfaces" label, and one of them is speculative dead code. It needs to be split before any part can merge.
What's actually in here (14 files, +754/-327)
- Dependency pin/lock refreshes (root
package.json/package-lock.json+ website + web + ui-tui + desktop) — legitimate security intent (React Router, DOMPurify), but see conflicts below. - launchd file-descriptor limits in
hermes_cli/gateway.py— plausibly useful, but unrelated to "UI surfaces." - 🔴
apps/desktop/src/lib/company-brain-trace.ts— a new trace parser/formatter primitive. It is imported nowhere (I checked: only its own test references it), and the PR description states it's "for future important-answer source/correction gating." This is speculative infrastructure with no current consumer — we don't merge hooks/primitives until something actually uses them. pane-shell.tsx— a resize-behavior change (drops thepane.resizableguard onwidthOverride), unrelated to security; plus misc TS cleanups in tui/website.
Blocking issues
- Speculative code (#3) should be dropped from this PR entirely. If the Company Brain gating feature is coming, land the primitive with its consumer in a dedicated feature PR.
- Conflicts:
mergeable: CONFLICTING/DIRTY. The rootpackage.json/package-lock.jsonedits also overlap several in-flight lockfile PRs — #44024 (@types/node, approved), #44197 (icons-pack, approved), and #39450 (website/whatsapp-bridge audit). These can't all merge independently; the dep work here should be reconciled with those rather than duplicating them. - Misleading label: a "chore" touching
gateway.pyservice config, adding a new desktop primitive, and changing pane resize behavior isn't a chore.
Ask
Please split into focused PRs, each rebased on current main:
- The dependency/lock refresh — but coordinate with #44024 / #44197 / #39450 so it's only the net-new remediation, not a re-do of what's already approved.
- The launchd FD-limit change (its own
fix(gateway)PR with the test you already wrote). - Drop
company-brain-trace.tsuntil it has a real consumer.
The pane-shell tweak, if intentional, belongs in whichever UI PR actually needs it. Happy to fast-track the launchd and dep pieces once separated.
|
Thanks, Austin. Split out the launchd piece into a focused PR from current main:
It contains only |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the security-maintenance work. The remaining useful pieces need to stay split rather than landing as this bundle.
Problems
apps/desktop/src/lib/company-brain-trace.tshas no production consumer in this PR: its only added import is its own test, while the body describes future gating.AGENTS.md:98-101rejects speculative infrastructure without a concrete consumer.- The
execFileNoThrowtyping change is already on current main in4ce9caed0(ui-tui/packages/hermes-ink/src/utils/execFileNoThrow.ts:1,35,37), and React Router was already remediated in46c16b928(apps/desktop/package.json:103,web/package.json:34). - The dependency work is not wholly obsolete: current
website/package-lock.json:9311still resolves DOMPurify 3.4.2, so any website remediation must be regenerated against current lockfiles.
Suggested changes
- Keep the launchd limits in focused PR #44357.
- Drop the unconsumed Company Brain parser and its tests.
- Re-scope remaining dependency work to unresolved current-main packages; retain the pane-shell change only as a separately justified desktop fix.
Automated hermes-sweeper review.
| @@ -0,0 +1,108 @@ | |||
| export interface CompanyBrainCorrection { | |||
There was a problem hiding this comment.
Please remove this parser from this PR. The diff adds no production import or consumer—only its own test—and the stated use is future gating; AGENTS.md:98-101 rejects speculative infrastructure without a concrete consumer.
| @@ -1,4 +1,4 @@ | |||
| import { spawn } from 'child_process' | |||
| import { spawn, type ChildProcess, type StdioOptions } from 'child_process' | |||
There was a problem hiding this comment.
This exact ChildProcess/StdioOptions typing change is already present on current main in commit 4ce9caed0 (fix(tui): type execFileNoThrow stdio/ChildProcess). Omit it from any salvage.
|
Closing this broad PR as superseded after maintainer review. The useful launchd file-descriptor change was split into focused PR #44357. I am dropping the unconsumed Company Brain parser and pane-shell change, and I will not carry forward the stale/overlapping dependency edits. Any future dependency remediation will be regenerated as a separate focused PR from current |
Summary
Verification
python -m pytest -o 'addopts=' tests/hermes_cli/test_gateway_service.py::TestLaunchdServiceRecovery::test_launchd_plist_sets_file_descriptor_limits tests/hermes_cli/test_gateway_service.py::TestServiceWorkingDirIsStable::test_launchd_plist_keepalive_unconditional -q(cd apps/desktop && npx vitest run --environment jsdom src/lib/company-brain-trace.test.ts src/components/pane-shell/pane-shell.test.tsx)npm --prefix ui-tui test -- --run packages/hermes-ink/src/utils/execFileNoThrow.test.tsnpm --prefix apps/desktop run type-checknpm --prefix ui-tui run type-checknpm --prefix website run typechecknpm audit --audit-level=moderate --json→ 0 vulnerabilitiesNotes
npm --prefix apps/desktop run test:ui -- company-brain-trace.test.tscurrently runs the whole desktopsrcsuite because the script includessrc; after merging latest main, that broader suite still has unrelated upstream UI test expectation failures. The targeted new/changed desktop tests pass via direct Vitest invocation above.