fix: approve transitive build scripts so pnpm install exits 0 - #135
Closed
cad0p wants to merge 53 commits into
Closed
fix: approve transitive build scripts so pnpm install exits 0#135cad0p wants to merge 53 commits into
cad0p wants to merge 53 commits into
Conversation
… markdown - Add markdown rendering path in renderBody with getMarkdownTheme() - Honor resultPreviewExpanded to bypass collapse for default UX - Add COLLAPSED_PREVIEW_LINES constant (10 lines, matching pi read/write precedent) - Implement collapse hint for markdown mode with line count
- Add resultPreviewMode toggle (markdown ↔ plain) - Add resultPreviewExpanded boolean toggle - Add failurePreviewMaxChars numeric input with 1-1048576 range - Update snapshotSettings, buildItems, and applyValue functions
…ale (CLEAN-1, CLEAN-2, CLEAN-3, SB-1, SB-2)
…st mocks (ORCH-1, COV-1, COV-2, COV-5, COV-6, SB-4) - Extract renderer functions to module scope for testability - Rewrite renderer-regression.test.ts to use actual subagentNotificationRenderer - Rewrite renderer-markdown.test.ts to use production render functions - Rewrite renderer-expanded-setting.test.ts to test actual renderer behavior - Rewrite menu-structure.test.ts to verify expected menu structure - Add COV-1: code fence cut mid-block test - Add COV-2: surrogate pair boundary truncation tests - Add COV-5: mixed success/failure status tests - Add COV-6: applySettings wiring tests - Fix SB-4: correct test comment about production default behavior
…, dedupe default constant)
- High surrogate at exact boundary: input ending at position 1999 (high surrogate of last emoji) drops unpaired surrogate, exercising safeTruncate's drop-trailing-high-surrogate path
- Isolated low surrogate: bare low surrogate in malformed input does not crash production
- No truncation needed: safeTruncate('hello', 5) returns 'hello' unchanged
- Cap zero: safeTruncate with cap=0 returns truncation message
Addresses R3-CORR-1, R3-COV-1, R3-COV-2
Both formatTaskNotification and buildNotificationDetails contained identical result preview logic with fallback chain (result ?? error ?? empty) and failure-mode truncation. Extracted to buildResultPreview() helper for DRY compliance and consistent behavior between XML and UI paths. Addresses R3-SB-2
- High surrogate at exact boundary: input ending at position 1999 (high surrogate of last emoji) drops unpaired surrogate, exercising safeTruncate's drop-trailing-high-surrogate path
- Isolated low surrogate: bare low surrogate in malformed input does not crash production
- No truncation needed: safeTruncate('hello', 5) returns 'hello' unchanged
- Cap zero: safeTruncate with cap=0 returns truncation message
Addresses R3-CORR-1, R3-COV-1, R3-COV-2
…cation The expect(true).toBe(true) assertion with comment 'Production default verified by grep: resultPreviewExpanded = true' is a tautology that doesn't verify anything about production code. Other tests in the same file already cover the resultPreviewExpanded behavior implicitly through override testing. Addresses R3-SB-1, R3-CLEAN-5, R3-COV-4
Rewritten test to create 15 lines with fence opening at line 8 and closing at line 14. Truncation at COLLAPSED_PREVIEW_LINES=10 now actually cuts the fence mid-block, leaving 3 lines of code visible without the closing fence marker. Addresses R3-SB-4
…ions Added snapshot variants where d.others is populated to verify group rendering paths: - 2 agents: completed + completed - 2 agents: completed + error (mixed) - 3 agents: completed + steered + aborted (all success-ish) Snapshots lock the byte-equivalent output for group rendering with different status combinations. Addresses R3-COV-3
Changed inline comment to explain that 1999 = (1000 emoji × 2 UTF-16 units) - 1, which cuts the LAST emoji's high surrogate and exercises safeTruncate's drop-trailing-high-surrogate path. This makes the boundary value meaningful rather than arbitrary. Addresses R3-SB-3
…S fallback (BLOAT-1, BLOAT-4)
…T-3, DOC-1, DOC-6)
…orts (DOC-3, DOC-10)
…ehavior" (BLOAT-5, DOC-8)
…field JSDoc (DOC-9)
…lent fallback (BLOAT-1) The previous BLOAT-1 commit (e260e35) only added parentheses; the speculative `?? DEFAULT_FAILURE_PREVIEW_MAX_CHARS` fallback survived. This pass applies the reviewer's option (c) — runtime assertion that surfaces the failure-cap contract instead of silently falling back to a duplicate default. DEFAULT_FAILURE_PREVIEW_MAX_CHARS remains in use as the module-scope initializer (load-order-required before applySettings runs), per BLOAT-4 clarification.
…VERTRIM-1) Commit 5cd8dda merged stacked /** descriptive */ /** @internal */ blocks into single /** @internal description */ form for the @internal testability exports. Three of five functions (formatTaskNotification, buildNotificationDetails, subagentNotificationRenderer) got the proper merge. The other two (subagentNotificationRenderHeader, subagentNotificationRenderBody) got their descriptions deleted instead of merged. Restore consistency by applying the same merged-form pattern.
…only (DOC2-1) The constant's role (module-scope initializer for the local failurePreviewMaxChars at the bottom of index.ts) is encoded by the symbol name and the single use site. The "mirrors settings.ts default until applySettings runs" framing is provenance, not contract. Reflagged across two rounds — the right fix is to drop the provenance half, not edit the comment further.
This file declares all-local literals and asserts on those literals. It does not import from src/ — a refactor that removed resultPreviewMode from production would still leave it green. Settings semantics are pinned by test/result-preview-settings.test.ts (real production import) and Phase 2 E2E scenario 5 (live menu wiring). No regression contract is lost by deleting this.
… (B2) The 4 UI-path it() blocks in notification-format.test.ts (calling buildNotificationDetails) duplicate identical assertions in notification-details.test.ts. The format file's stated scope is formatTaskNotification (XML payload) — UI path was scope drift. R3 surrogate boundary findings remain pinned by the originals in notification-details.test.ts.
…ed tests (B3) test/renderer-expanded-setting.test.ts had 5 tests that all asserted length === 2 with single-line resultPreview fixtures — they pass identically whether the renderer is in expanded or collapsed mode. A future refactor that inverted the effectiveExpanded ternary would still leave them green. Replace with 2 tests in renderer-markdown.test.ts using a 50-line resultPreview, asserting hint presence/absence in markdown.text to actually distinguish the branches. Real regression locks instead of smoke tests.
The handles-very-large-input-without-performance-issues test asserted duration < 100ms against 1MB ASCII operations — a magic threshold not backed by any reviewer-pinned source, flaky on shared CI runners under load. Large-input behavior is covered structurally by the 100KB tests in notification-details.test.ts.
…ion.test.ts (B8) The structural-properties-remain-consistent test asserted children.length === 2, but the two toMatchInlineSnapshot tests above already lock the rendered shape. Comment claimed structural test was needed for "components that can't be rendered to text" — the snapshot tests in fact rendered to text via extractRenderedText, so the rationale was stale.
…act (B9) The empty body + markdown mode renders sensibly test asserted container.children.length >= 0 — unconditionally true. The genuine no-crash contract is now expressed as expect(...).not.toThrow(), making the assertion actually testing what the test name claims.
…MaxChars (R2-T1) Commit 84cc4fb added a runtime contract assertion in buildResultPreview that throws when failurePreviewMaxChars is undefined on error/stopped status. Every existing failure-status test fixture pairs status with a numeric setting, so the throw path was reachable but uncovered. This test pins the contract.
…on-format.test.ts (TR3-T1) B2 (398b77e) deleted 4 of the 5 duplicate UI-path tests in notification-format.test.ts but missed `buildNotificationDetails handles surrogate pairs in UI path`. Its byte-equivalent counterpart is at test/notification-details.test.ts:94 (it("UI path: surrogate pairs handled gracefully", ...)). The contract (UI-path surrogate-pair handling at boundary slice) remains pinned by the surviving copy plus the analogous XML-path test in notification-format.test.ts:18. Closes B2 cleanly.
feat: markdown result preview + failure-body fallback + uncapped success output
… loader-level extensions, prompt KV-cache reorder, glyph fix
Eject general-purpose, Explore, Plan as .pi/agents/*.md and widen auditor. All four use tools: "*" (all built-ins) + extensions: true with no ext: selectors, so each agent surfaces every built-in and extension tool. Explore/Plan keep their guidance prompts but drop the read-only restriction.
The full-tool-access built-in agents (general-purpose, Explore, Plan) are user-global config, not repo config — committing them here only affected work done inside this repo. Moved to ~/.pi/agent/agents/ instead. Restores auditor.md to its original demo state. Reverts 8efe92d.
….10.0→v0.12.0) (#2)
pnpm 11 blocks native build scripts by default and exits non-zero ([ERR_PNPM_IGNORED_BUILDS]) when any are unapproved. Two transitive deps pulled in through the @earendil-works/pi-* peers ship install scripts (@google/genai, protobufjs). Without approval, pnpm install returns a failing exit code, which breaks pi's git-package reconciliation (git clean -fdx -> pnpm install -> non-zero), leaving the extension unable to load (e.g. "Cannot find module 'croner'"). Add a pnpm-workspace.yaml with an allowBuilds entry (the pnpm 11 replacement for the removed onlyBuiltDependencies setting) so pnpm install exits 0. Verified: install succeeds, croner resolves, file survives git clean -fdx because it is tracked.
Author
|
Closing — opening against my fork instead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
With pnpm 11 as the configured package manager (
"npmCommand": ["pnpm"]in pi settings), installing this package as a pi git package fails to load:Root cause
pi reconciles git packages with
git clean -fdxthen<npmCommand> install. Two transitive dependencies pulled in through the@earendil-works/pi-*peers ship native build scripts:@google/genaiandprotobufjs.pnpm 11 blocks unapproved build scripts by default and exits non-zero (
[ERR_PNPM_IGNORED_BUILDS]) when any are present. Sopnpm installreturns a failing exit code even though the dependencies themselves are installed correctly. pi'srunCommandtreats the non-zero exit as a failure, which breaks reconciliation and leaves the extension unable to load.Fix
Add a
pnpm-workspace.yamlwith anallowBuildsentry — the pnpm 11 replacement for the removedonlyBuiltDependenciessetting — approving the two transitive packages with build scripts.pnpm installnow exits 0.The file is tracked, so it survives pi's
git clean -fdxreconcile step.Verification
pnpm installexits 1 (ERR_PNPM_IGNORED_BUILDS).cronerresolves, extension loads.npm run lintandnpm run typecheckpass.