Conversation
Security review follow-up — e05f8ddAddressed the five findings reported on the initial head:
No CodeQL alert was dismissed or suppressed. Dependency impactThe PR body and Verification
The PR remains a draft for maintainer scope/architecture review; the skipped Codex PR Review job is not a human approval. |
There was a problem hiding this comment.
Findings
-
[Minor] Research guidance is injected even when no research tools exist —
packages/core/src/agent.ts:1216buildsbaseAgenticGuidanceby unconditionally prependingWEB_RESEARCH_GUIDANCE, while the tools are only registered underif (deps.research)(packages/core/src/agent.ts:1194) andavailableToolNames({ research: deps.research !== undefined, ... }). Callers that omitresearchstill receive a system prompt telling the model to putdata-slide-idon every section and to callresearch_export(path)after the final page edits, which invites calls to tools that are not registered (and, on a plain deck, companion files made only ofEvidence gapentries).
Suggested fix: only prepend the block whendeps.researchis defined, and add a Vitest case assertingWEB_RESEARCH_GUIDANCEtext is absent fromstate.systemPromptwhenresearchis not passed. -
[Minor] A companion-file failure now aborts the primary export —
apps/desktop/src/main/exporter-ipc.ts:260awaitsprepareResearchExport(resolved)beforeexportArtifactwith no guard. That path runsreadResearchSlides(RESEARCH_SLIDES_SCRIPTthrows on canvas/iframe/video slides, on a section without a uniquedata-slide-id, and on a deck that failed to render) andloadResearchStore(throws on a malformed.codesign/research.json). A design that exported fine before can now fail HTML/PDF/PPTX/ZIP/Markdown export entirely because of optional research metadata, with an error worded for research slides rather than for the export the user requested.
Suggested fix: catch companion-generation errors, keep the export result, and append the message toresearchWarningsso it is still surfaced in the existing export toast; or only hard-fail when the user explicitly asked for a sources file. -
[Nit]
research_recordswrites the file it only reads —apps/desktop/src/main/web-research.ts:120routesreadRecordsthroughtransaction(), which always callssaveResearchStore, so a read-onlyresearch_recordscall rewrites (and creates, when absent).codesign/research.json.
Suggested fix: add a read-only host path that skipssaveResearchStorewhen nothing changed.
Questions
- Was every remaining
writeConfigcall site that builds a freshConfigliteral audited forwebSearchpreservation? The diff covers provider CRUD, Codex OAuth, external imports, image settings, design-system saves and onboarding reset, andapps/desktop/src/main/onboarding-ipc.test.tsexercises those. Any unaudited save path that still rebuilds the config would silently drop the opt-in and limits on the next ordinary save.
Summary
- Review mode: initial. No blockers found; the two Minor items above are the only material concerns, and both are conditional on the research feature being in use.
- Scope and constraints: the change is additive and offline-safe by default.
parse5is imported lazily insidereadableHtml(apps/desktop/src/main/web-research-network.ts), so startup does not load it;packages/exporters/src/index.ts:18lazy-imports the slide reader. New deps are permissive (parse5 8.0.1 MIT,entities8.1.0 BSD-2-Clause) andpnpm-lock.yamlrecordsentitiesenginesnode >=20.19.0, compatible withpackage.jsonenginesnode >=22. No provider SDK imports were added; LLM calls still route through the existing agent path. - Security posture looks sound from the diff: public HTTP(S) only on ports 80/443, no embedded credentials, reserved/private/loopback/link-local/CGNAT/IPv4-mapped IPv6 ranges blocked, all DNS answers validated before connect, the socket pinned to the validated address, every redirect re-validated, 1 MiB body cap, redirect cap, per-run call budget, timeout and cancellation, and sanitized error messages that do not echo the API key. HTML is parsed as untrusted text, never inserted as markup. Consent is genuinely host-enforced through the existing ask IPC and is per-run (
apps/desktop/src/main/web-research.ts:96). - Credential-migration change is consistent with the no-silent-fallback rule: unreadable entries are preserved with a credential-free log line, and
decryptSecretstill fails strictly on use(apps/desktop/src/main/keychain.ts:96. Error-message narrowing indecryptSafeStorageis guidance-only. - The linked issue is referenced as
Refs #432, which is the correct form for a partial/first slice; no closure claim needed validating. Changesets cover the four affected packages, docs and README were updated, and no release/packaging/distribution files are touched, so there is no checksum/manifest path to verify. - Residual risk: the PR body states live Tavily and a live-model autonomous E2E were not run in that session, and I could not independently verify the reported CI/CodeQL results from this diff. Windows-first validation plus a pinned Node 22 run remains a gap the author already flags.
Testing
- Not run (automation). Suggested additions: a Vitest assertion that the research guidance and
data-slide-idinstructions are absent whendeps.researchis undefined; a test that a corrupt.codesign/research.jsonor a non-conforming deck does not fail the primary export (or that the failure is surfaced throughresearchWarnings); and a test thatresearch_recordsdoes not create or rewrite.codesign/research.json.
Open-CoDesign Bot
There was a problem hiding this comment.
Findings
No new findings. The three items from the previous review (e05f8dd) are all addressed by this push:
- [Resolved] Research guidance was injected without research tools —
packages/core/src/agent.ts:1215now buildsresearchGuidanceonly whenresearchTools.length > 0and every research tool is present in the finaltoolslist, andpackages/core/src/agent.test.tsadds the two cases (no host; explicittools: []override) asserting## Slides research and separate sources,data-slide-id, andresearch_exportare absent from the prompt. - [Resolved] Companion failure aborted the primary export —
apps/desktop/src/main/exporter-ipc.ts:257wrapsprepareResearchExportand thewriteUniqueSourcescall (:274) intry/catch, converts failures intoresearchWarnings, and still returnsstatus: 'saved'.apps/desktop/src/main/exporter-ipc.research.test.tsasserts the corrupt-store and render/prepare failures keep the primary file for all five formats, while primary exporter errors still reject (does not suppress primary exporter failures). - [Resolved]
research_recordsrewrote the file it only reads —apps/desktop/src/main/web-research.ts:100now uses a read-onlyinWorkspacepath with nosaveResearchStore, andapps/desktop/src/main/web-research-store.ts:36gates.codesigncreation behindstorePath(root, true)used only bysaveResearchStore.apps/desktop/src/main/web-research.test.ts(read-only research records) verifies no directory/file creation and unchanged bytes/mtime.
Questions
None.
Summary
- Review mode: follow-up after new commits.
- All three prior findings are fixed in the diff and covered by targeted Vitest cases; I did not find a new correctness, security, or regression issue introduced by this push. The prior Minor items are no longer merge-relevant.
- Constraint posture is unchanged and sound:
parse5remains lazily imported insidereadableHtml(apps/desktop/src/main/web-research-network.ts), so startup does not load it; the new dependency is MIT (parse58.0.1) with a BSD-2-Clause transitive (entities8.1.0), andpnpm-lock.yamlpinsentitiesenginesnode >=20.19.0, compatible with the repo'snode >=22. No provider SDK imports were added; LLM calls still route through the existingpi-ai/agent path. The SSRF boundary (public HTTP(S) only, ports 80/443, no embedded credentials, reserved/private/loopback/link-local/CGNAT/IPv4-mapped IPv6 blocked, all DNS answers validated and the socket pinned, per-redirect revalidation, 1 MiB cap, redirect/call budgets, timeout and cancellation, sanitized errors) is unchanged from the previously reviewed state. - The linked issue is referenced as
Refs #432, which is correct for this first slice, so no closure claim needed validating. Changesets cover the affected packages and no release/packaging/distribution files are touched. - Residual risk (unchanged, author-acknowledged): live Tavily and a live-model autonomous end-to-end run were not verified in this session, and I cannot independently reproduce the reported remote CI/CodeQL results from the diff. Node 22 and non-Windows packaging remain unverified gaps.
Testing
- Not run (automation). The new coverage is proportional:
apps/desktop/src/main/exporter-ipc.research.test.ts:79(primary-export resilience across all formats),apps/desktop/src/main/web-research.test.ts:303(read-only records), andpackages/core/src/agent.test.ts:2710(guidance/tool gating). A live-provider smoke test on Node 22 would close the remaining confidence gap, but that is environment work rather than a code change.
Open-CoDesign Bot
Summary
Add opt-in Web Search v1 for research-backed slide decks: search and read public references, save evidence while working, associate it with stable slide IDs, and generate an independent Markdown sources document through normal exports. Slides do not get source footers, citation numbers, chart source captions, or a references page by default.
Draft for scope and architecture review. This is a larger first-version change (including tests and documentation), not a request to bypass the project's small-PR preference. The supporting startup fix is isolated in its own commit and can be split/cherry-picked if preferred.
Type of change
Linked issue
Refs #432 — Opt-in web research for slides with independent source documents.
Implementation
web_search,web_fetch, and small research-record/evidence/slide/export tools through the existing manifest and final model-visible tool list. No agent-loop rewrite or MCP framework.parse5text-node extraction, not regex stripping or script execution. Output is untrusted text, not sanitized HTML or instructions..codesign/research.jsonusing the existing workspace writer lock and atomic replacement. Reject dangling IDs and quotations not present in saved excerpts; mark reading separately from fact verification.data-slide-idand rendered semantic fingerprints rather than page numbers. Reorder/deletion follows current DOM order; changed content yields evidence gaps instead of old supporting links. Styling-only changes such as color/font changes retain evidence.Configuration and usage
See
WEB_SEARCH.md. With the app closed, add/update the active local config:Restart, submit a research-backed slide request, and approve Allow this run when prompted. The key is never a tool parameter or renderer-facing setting. Feature remains disabled by default; configuring a key alone does not enable networking.
Validation
Rebased onto upstream
ed5fed9(v0.2.2-era main) and adapted research consent to the new persisted question/run scope. The normal pre-commit and pre-push hooks were used, not bypassed.pnpm lintpnpm -r typecheck(normal pre-push hook)pnpm testpnpm --filter @open-codesign/desktop buildgit diff --checkLocal validation environment: Windows, Node 24.11.1, pnpm 10.33.4, installed system browser. The repository pins Node 22; a separate Node 22 local run and installer packaging were not performed. Remote CI, CodeQL and Linux packaging smoke have now passed for
e05f8dd; maintainer review is still required.Commits:
21ddaf1— keep unreadable credentials from blocking startup (independently reviewable supporting fix).198731c— opt-in web research and slide source exports.e05f8dd— security review remediation: parsed HTML text extraction and linear EDITMODE scanning.Automated integration uses mocked search/fetch responses but real system-browser HTML/JSX rendering and ZIP output, including record recovery, page reorder/deletion, visual-only changes without repeat searches, stale evidence, nested asset paths, and concurrent workspace writes. Network-boundary tests cover normalization, missing fields, private/metadata/mapped IPs, pinned DNS, redirects, budgets, failure, timeout, cancellation, and output limits.
A local tester reports successful Tavily search and basic slide usage. I have not independently run a comprehensive real-provider/autonomous-live-model E2E or tested every target OS. No credentials, private workspace records, generated artifacts, or local validation logs are included.
First-version limits
Dependency additions and security-review follow-up
The initial no-dependency HTML stripper was replaced in response to CodeQL alerts #188–191. No alert was dismissed or suppressed.
parse5parses without executing scripts or requesting resources; an iterative tree walk reads text nodes and skips active/non-content subtrees. Encoded tag literals remain untrusted text, not HTML to insert.parse58.0.1entities8.1.0Total: 667,290 bytes (~652 KiB); not a measured installer delta. Verified package licenses and dependency paths with
pnpm why. Existing parse5 5/6 copies are incidental transitive/dev dependencies; explicitly declaring the maintained, typed ESM parser avoids relying on those in a packaged app. Lightweight string helpers are not HTML5 parsers, and invoking a browser would introduce execution/resource-loading concerns. A peer dependency would leave the shipped reader dependent on a separate user install.The fifth CodeQL annotation identifies polynomial runtime EDITMODE matching on repeated BEGIN markers without END. It is now a forward-only comment/marker scan, with complete-block compatibility and a 50,000-marker regression. The change is limited to the flagged binding path.
Review follow-up validation: parser/network tests (37), runtime tests (94), research/consent integration (included in a 72-test focused desktop run), desktop typechecks and build pass. The normal pre-push hooks also passed: 3,281 package tests plus 61 script tests, 4 skips, workspace typechecks and lint. Remote CI, Dependency Review, CodeQL and Linux packaging smoke all passed for
e05f8dd; CodeQL reports no new alerts and zero annotations (see the follow-up comment).Compatibility / upgradeability / lean / clarity
Checklist
Screenshots / recordings
No new panel or visual layout is introduced. Consent reuses the existing question dialog, and companion-file paths use existing export notifications. No screenshot/recording is attached; this remains a draft.