refactor: strengthen repository maintenance boundaries - #34
Merged
Conversation
Centralize web tool limits and cancellation behavior, make the network policy auditable, and add compatibility, release cleanup, and repository contract checks. Document architecture and manual dependency review.
Contributor
📝 WalkthroughWalkthroughThe PR adds Pi compatibility CI, expands repository documentation and contracts, centralizes web-fetch and web-search limits, strengthens cancellation and validation behavior, and refactors release automation for injection and testing. ChangesRepository maintenance and package hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
packages/pi-web-search/src/brave.ts (1)
24-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate request validation across
brave.tsandsearch.ts. Both files independently enforce the same count-bounds and mode-specific query-length rules against the sameSEARCH_MIN_RESULT_COUNT/SEARCH_MAX_RESULT_COUNT/SEARCH_CONTEXT_MAX_QUERY_CHARACTERS/SEARCH_WEB_MAX_QUERY_CHARACTERSconstants, but with different error message wording depending on which entry point a caller uses.
packages/pi-web-search/src/brave.ts#L24-L42: keepvalidateProviderRequestas the single source of truth for these checks (or move it tolimits.ts), and export it for reuse.packages/pi-web-search/src/search.ts#L98-L116: replace the inline count and query-length checks inSearchRuntime.executewith a call to the shared validation function frombrave.ts/limits.ts, so both entry points produce identical error messages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi-web-search/src/brave.ts` around lines 24 - 42, Make validateProviderRequest the shared source of truth by exporting it from packages/pi-web-search/src/brave.ts (or moving it to limits.ts) while preserving its count and mode-specific query-length checks. In packages/pi-web-search/src/search.ts lines 98-116, remove the duplicated validation in SearchRuntime.execute and call the shared function so both entry points use identical error messages.packages/pi-web-search/src/index.ts (1)
60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWeb query description omits its character limit.
The context-query description interpolates
SEARCH_CONTEXT_MAX_QUERY_CHARACTERSinto its text, but the web-query description at Line 64 stays a static"The web search query"without mentioningSEARCH_WEB_MAX_QUERY_CHARACTERS. Align the two descriptions for consistency.✏️ Proposed fix
query: Type.String({ minLength: 1, maxLength: SEARCH_WEB_MAX_QUERY_CHARACTERS, - description: "The web search query", + description: `The web search query (maximum ${SEARCH_WEB_MAX_QUERY_CHARACTERS} characters)`, }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi-web-search/src/index.ts` around lines 60 - 65, Update the web query field description in the Type.Object schema to include the SEARCH_WEB_MAX_QUERY_CHARACTERS limit, matching the context-query description’s format while preserving the existing validation.packages/pi-web-search/tests/harness.ts (1)
10-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest harness redefines production types instead of importing them.
SearchParameters,SearchExecutionResult, andSearchToolare hand-written here rather than imported from../src/searchor../src/index. If the real parameter or details shape changes, these local interfaces stay unchanged and tests keep compiling and passing against a stale contract.Import and reuse the actual exported types where possible, to keep the harness structurally tied to the production contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi-web-search/tests/harness.ts` around lines 10 - 63, Replace the locally defined SearchParameters, SearchExecutionResult, and SearchTool interfaces in the test harness with imports of the corresponding exported production types from the package’s search or index modules. Reuse the existing RenderedComponent and RenderTheme definitions only if they are not exported, and update harness references to preserve the production contract.scripts/release.ts (2)
84-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSurface
spawnSyncspawn failures inattempt.
spawnSyncsetserrorand leavesstatusasnullwhen the binary is missing or cannot start.attemptdrops that error, sostderrstays empty. Callers then throw messages with no reason, for exampleUnable to check npm for@zeldrisho/alpha@1.0.0:.Include the spawn error in
stderr.♻️ Proposed change
return { status: result.status, stdout: result.stdout ?? "", - stderr: result.stderr ?? "", + stderr: result.stderr ?? (result.error ? `${result.error.message}` : ""), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release.ts` around lines 84 - 96, Update attempt to include result.error in the returned stderr when spawnSync fails to start the command, while preserving any existing stderr output. Ensure callers receive the spawn failure reason instead of an empty message when result.status is null.
302-312: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valuePartial state remains when the changelog step fails.
preparewrites the bumped manifest first. If thegit-cliff --prependcall then throws, the manifest holds the new version and the changelog holds the old content. A laterstatusorpreparerun reports the manifest as inconsistent with the latest component tag.The release workflow runs this on a throwaway checkout, so recovery is a re-run. If you want the failure to be self-clearing, write the manifest after the changelog succeeds, or restore the manifest in a
catchblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release.ts` around lines 302 - 312, Update the prepare flow around the manifest write and git-cliff invocation so a failed changelog step does not leave the manifest at the bumped version. Move the manifest update and write after the git-cliff call succeeds, or restore the original manifest in a catch block, while preserving the existing version and changelog behavior on success.scripts/release.test.ts (2)
32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrite files with
node:fsinstead of spawningnode -e. Both helpers start a child Node process only to write one file. The same work is available in-process, which is faster and removes the nested string escaping. Thecommithelper also writes the constant content'x', so a second call on the same repository stages no change andgit commitexits non-zero.
scripts/release.test.ts#L32-L39: makecommitasync, replace thenode -ecall withawait writeFile(marker, ...), and write unique content so repeated calls always produce a commit. Update the three call sites toawait.scripts/release.test.ts#L54-L65: replace thenode -ecall in the--prependbranch withwriteFileSyncfromnode:fs, becauseCommandRunner.runis synchronous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release.test.ts` around lines 32 - 39, Update scripts/release.test.ts:32-39 by making commit asynchronous, using node:fs writeFile with await instead of spawning node -e, and writing unique content on each call so repeated commits succeed; update all three commit call sites to await it. At scripts/release.test.ts:54-65, replace the synchronous node -e file write in the --prepend branch with writeFileSync imported from node:fs, preserving CommandRunner.run’s synchronous behavior.
172-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
runReleaseCliand the no-changes branch.Two exported behaviors have no test:
runReleaseClidispatch, including the missing-argument error forensure-github-releaseand the usage error for an unknown command.- The
prepare()path that logs"No releasable package changes found.".
runReleaseClicallscreateReleaseAutomation()with no options, so a test must run against the real repository root. Consider adding an options parameter torunReleaseClito make it injectable, or test only the argument-validation errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release.test.ts` around lines 172 - 184, Add tests in scripts/release.test.ts covering runReleaseCli command dispatch, including its missing-argument error for ensure-github-release and usage error for unknown commands; make the automation injectable if needed, or limit tests to argument validation. Also add coverage for createReleaseAutomation.prepare() when there are no releasable package changes, asserting it logs "No releasable package changes found.".packages/pi-web-fetch/src/index.ts (1)
37-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInterpolate
FETCH_DEFAULT_OFFSETinstead of hardcoding0in the description.The
offsetdescription hardcodes the text(default: 0; ...). ThemaxCharactersfield below correctly interpolatesFETCH_DEFAULT_MAX_CHARACTERS. ImportFETCH_DEFAULT_OFFSETfrom./limitsand use it here for consistency, so the documented default cannot drift from the runtime default inservice.ts.♻️ Proposed fix
import { FETCH_DEFAULT_MAX_CHARACTERS, + FETCH_DEFAULT_OFFSET, FETCH_MAX_CHARACTERS, FETCH_MAX_OFFSET_CHARACTERS, FETCH_MAX_URL_CHARACTERS, FETCH_MIN_MAX_CHARACTERS, } from "./limits"; ... offset: Type.Optional( Type.Integer({ minimum: 0, maximum: FETCH_MAX_OFFSET_CHARACTERS, description: - "Extracted-content character offset to start reading from (default: 0; use nextOffset to continue)", + `Extracted-content character offset to start reading from (default: ${FETCH_DEFAULT_OFFSET}; use nextOffset to continue)`, }), ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi-web-fetch/src/index.ts` around lines 37 - 44, Update the offset schema description near the offset field to interpolate FETCH_DEFAULT_OFFSET instead of hardcoding 0, and import FETCH_DEFAULT_OFFSET from ./limits. Keep the existing description wording and runtime behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 22: Set persist-credentials to false on all six actions/checkout@v7 steps
in .github/workflows/ci.yml (22-22), .github/workflows/pi-compatibility.yml
(25-25), and .github/workflows/release.yml (19-19, 38-38, 67-67, 98-98). In the
prepare workflow, provide temporary Git authentication only around the two git
push commands, without leaving credentials available to vp install or other
scripts.
In `@docs/architecture.md`:
- Line 69: In the API-key timing sentence, replace the ambiguous “read only at
request time” wording with “read at request time only,” while preserving the
statements about cache keys, output, errors, and Web mode.
- Around line 46-51: The architecture documentation’s blocked-address policy
paragraph lacks an auditable registry link and review metadata. Update the
paragraph to include a direct authoritative IANA special-purpose registry
reference and the review date, or explicitly point readers to the table comment
in network-policy.ts containing that metadata; preserve the existing security
requirements.
In `@docs/plan.md`:
- Around line 9-15: Update the baseline metrics in the plan around the audit
results: either add the audit date and explicitly label the 137-test and
coverage figures as historical, or replace them with the current 232-test,
94.44% line-coverage, and 82.06% branch-coverage values.
In `@packages/pi-web-fetch/src/extract.ts`:
- Around line 13-15: Update the replacement-generation logic around
replacementIndex so each generated defuddle-safe-id is checked against all IDs
already present in the document and skipped when occupied; preserve the existing
replacements mapping and normalized element assignment. Add a regression fixture
containing both an existing defuddle-safe-id-0 and the element requiring
normalization, verifying the fragment link targets the normalized element.
In `@packages/pi-web-fetch/src/network-policy.ts`:
- Around line 30-42: Update BLOCKED_IPV6_RANGES in
packages/pi-web-fetch/src/network-policy.ts (lines 30-42) to block 2001::/23,
100:0:0:1::/64, 3fff::/20, and 5f00::/16, while explicitly permitting only the
documented globally reachable /128 exceptions within 2001::/23. In
packages/pi-web-fetch/tests/network-policy.test.ts (lines 45-50), change the
2001:1:ffff:... expectation to blocked and add boundary coverage for each new
blocked range and every explicit exception.
In `@scripts/release.ts`:
- Around line 155-161: Escape the literal package directory before interpolating
it into regexes. Update componentTags to use the escaped pkg.directory when
constructing its pattern, and apply the same escaping to the package-directory
portion of the pattern built by cliffArguments for --tag-pattern; preserve
existing tag matching behavior.
- Around line 341-345: Update the target resolution in the release flow around
tagged, target, and range so the fallback HEAD value is resolved to the full
commit SHA using git rev-parse before it is passed to gh release create.
Preserve existing pkg.tag and GITHUB_SHA behavior, and ensure the resolved
target is used consistently for the release range.
---
Nitpick comments:
In `@packages/pi-web-fetch/src/index.ts`:
- Around line 37-44: Update the offset schema description near the offset field
to interpolate FETCH_DEFAULT_OFFSET instead of hardcoding 0, and import
FETCH_DEFAULT_OFFSET from ./limits. Keep the existing description wording and
runtime behavior unchanged.
In `@packages/pi-web-search/src/brave.ts`:
- Around line 24-42: Make validateProviderRequest the shared source of truth by
exporting it from packages/pi-web-search/src/brave.ts (or moving it to
limits.ts) while preserving its count and mode-specific query-length checks. In
packages/pi-web-search/src/search.ts lines 98-116, remove the duplicated
validation in SearchRuntime.execute and call the shared function so both entry
points use identical error messages.
In `@packages/pi-web-search/src/index.ts`:
- Around line 60-65: Update the web query field description in the Type.Object
schema to include the SEARCH_WEB_MAX_QUERY_CHARACTERS limit, matching the
context-query description’s format while preserving the existing validation.
In `@packages/pi-web-search/tests/harness.ts`:
- Around line 10-63: Replace the locally defined SearchParameters,
SearchExecutionResult, and SearchTool interfaces in the test harness with
imports of the corresponding exported production types from the package’s search
or index modules. Reuse the existing RenderedComponent and RenderTheme
definitions only if they are not exported, and update harness references to
preserve the production contract.
In `@scripts/release.test.ts`:
- Around line 32-39: Update scripts/release.test.ts:32-39 by making commit
asynchronous, using node:fs writeFile with await instead of spawning node -e,
and writing unique content on each call so repeated commits succeed; update all
three commit call sites to await it. At scripts/release.test.ts:54-65, replace
the synchronous node -e file write in the --prepend branch with writeFileSync
imported from node:fs, preserving CommandRunner.run’s synchronous behavior.
- Around line 172-184: Add tests in scripts/release.test.ts covering
runReleaseCli command dispatch, including its missing-argument error for
ensure-github-release and usage error for unknown commands; make the automation
injectable if needed, or limit tests to argument validation. Also add coverage
for createReleaseAutomation.prepare() when there are no releasable package
changes, asserting it logs "No releasable package changes found.".
In `@scripts/release.ts`:
- Around line 84-96: Update attempt to include result.error in the returned
stderr when spawnSync fails to start the command, while preserving any existing
stderr output. Ensure callers receive the spawn failure reason instead of an
empty message when result.status is null.
- Around line 302-312: Update the prepare flow around the manifest write and
git-cliff invocation so a failed changelog step does not leave the manifest at
the bumped version. Move the manifest update and write after the git-cliff call
succeeds, or restore the original manifest in a catch block, while preserving
the existing version and changelog behavior on success.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a359fa4f-f68f-4adb-b281-a7679f54ab46
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
.github/workflows/ci.yml.github/workflows/pi-compatibility.yml.github/workflows/release.ymlAGENTS.mdREADME.mddocs/architecture.mddocs/development.mddocs/plan.mddocs/releases.mdpackages/pi-web-fetch/package.jsonpackages/pi-web-fetch/src/abort.tspackages/pi-web-fetch/src/extract.tspackages/pi-web-fetch/src/fetch.tspackages/pi-web-fetch/src/index.tspackages/pi-web-fetch/src/limits.tspackages/pi-web-fetch/src/network-policy.tspackages/pi-web-fetch/src/network-redirects.tspackages/pi-web-fetch/src/service.tspackages/pi-web-fetch/tests/abort.test.tspackages/pi-web-fetch/tests/caching.test.tspackages/pi-web-fetch/tests/cancellation.test.tspackages/pi-web-fetch/tests/extraction.test.tspackages/pi-web-fetch/tests/harness.tspackages/pi-web-fetch/tests/index.test.tspackages/pi-web-fetch/tests/limits.test.tspackages/pi-web-fetch/tests/network-policy.test.tspackages/pi-web-fetch/tests/redirects.test.tspackages/pi-web-fetch/tests/transport.test.tspackages/pi-web-search/src/brave.tspackages/pi-web-search/src/index.tspackages/pi-web-search/src/limits.tspackages/pi-web-search/src/search.tspackages/pi-web-search/tests/caching-coalescing.test.tspackages/pi-web-search/tests/context-formatting.test.tspackages/pi-web-search/tests/harness.tspackages/pi-web-search/tests/index.test.tspackages/pi-web-search/tests/limits.test.tspackages/pi-web-search/tests/provider-transport.test.tspackages/pi-web-search/tests/schema-rendering.test.tspackages/pi-web-search/tests/truncation-lifecycle.test.tsscripts/package-smoke-test.tsscripts/release.test.tsscripts/release.tsscripts/repository-contract-test.ts
💤 Files with no reviewable changes (2)
- packages/pi-web-fetch/tests/index.test.ts
- packages/pi-web-search/tests/index.test.ts
Contributor
|
Note Docstrings generation - SUCCESS |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
Docstrings generation was requested by @zeldrisho. The following files were modified: * `packages/pi-web-fetch/src/abort.ts` * `packages/pi-web-fetch/src/extract.ts` * `packages/pi-web-fetch/src/fetch.ts` * `packages/pi-web-fetch/src/network-redirects.ts` * `packages/pi-web-fetch/src/service.ts` * `packages/pi-web-fetch/tests/harness.ts` * `packages/pi-web-search/src/brave.ts` * `packages/pi-web-search/tests/harness.ts` * `scripts/package-smoke-test.ts` * `scripts/release.ts` These files were ignored: * `packages/pi-web-fetch/tests/abort.test.ts` * `packages/pi-web-fetch/tests/caching.test.ts` * `packages/pi-web-fetch/tests/cancellation.test.ts` * `packages/pi-web-fetch/tests/extraction.test.ts` * `packages/pi-web-fetch/tests/limits.test.ts` * `packages/pi-web-fetch/tests/network-policy.test.ts` * `packages/pi-web-fetch/tests/redirects.test.ts` * `packages/pi-web-fetch/tests/transport.test.ts` * `packages/pi-web-search/tests/caching-coalescing.test.ts` * `packages/pi-web-search/tests/context-formatting.test.ts` * `packages/pi-web-search/tests/limits.test.ts` * `packages/pi-web-search/tests/provider-transport.test.ts` * `packages/pi-web-search/tests/schema-rendering.test.ts` * `packages/pi-web-search/tests/truncation-lifecycle.test.ts` * `scripts/release.test.ts` These file types are not supported: * `.github/workflows/ci.yml` * `.github/workflows/pi-compatibility.yml` * `.github/workflows/release.yml` * `AGENTS.md` * `README.md` * `docs/architecture.md` * `docs/development.md` * `docs/plan.md` * `docs/releases.md` * `packages/pi-web-fetch/package.json`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pi-web-fetch/src/network-policy.ts`:
- Around line 47-61: Update the IPv6 allowlist initialization near
GLOBALLY_REACHABLE_IPV6_EXCEPTIONS to add subnets for 2001:3::/32,
2001:4:112::/48, 2001:20::/28, and 2001:30::/28 via
allowedIPv6Addresses.addSubnet, then add boundary tests verifying
validateRemoteUrl accepts addresses within each prefix while preserving
rejection outside them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: febe81b5-18ae-4540-bd6c-885ab8fbb520
📒 Files selected for processing (22)
.github/workflows/ci.yml.github/workflows/pi-compatibility.yml.github/workflows/release.ymldocs/architecture.mddocs/plan.mdpackages/pi-web-fetch/src/abort.tspackages/pi-web-fetch/src/extract.tspackages/pi-web-fetch/src/fetch.tspackages/pi-web-fetch/src/index.tspackages/pi-web-fetch/src/network-policy.tspackages/pi-web-fetch/src/network-redirects.tspackages/pi-web-fetch/src/service.tspackages/pi-web-fetch/tests/extraction.test.tspackages/pi-web-fetch/tests/harness.tspackages/pi-web-fetch/tests/network-policy.test.tspackages/pi-web-search/src/brave.tspackages/pi-web-search/src/index.tspackages/pi-web-search/src/search.tspackages/pi-web-search/tests/harness.tsscripts/package-smoke-test.tsscripts/release.test.tsscripts/release.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/pi-web-search/src/index.ts
- packages/pi-web-fetch/src/network-redirects.ts
- packages/pi-web-fetch/src/fetch.ts
- packages/pi-web-fetch/src/extract.ts
- packages/pi-web-fetch/tests/network-policy.test.ts
- packages/pi-web-fetch/src/index.ts
- packages/pi-web-search/src/search.ts
- scripts/package-smoke-test.ts
- packages/pi-web-fetch/tests/extraction.test.ts
- docs/architecture.md
- packages/pi-web-fetch/src/service.ts
- packages/pi-web-fetch/src/abort.ts
- packages/pi-web-search/tests/harness.ts
- .github/workflows/pi-compatibility.yml
- docs/plan.md
- .github/workflows/ci.yml
This was referenced Aug 10, 2026
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
1 participant
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.
Summary
Validation
vp run validateSummary by CodeRabbit