fix(config): honor documented PI_RULES_* env vars - #25
Open
Yoonkeee wants to merge 6 commits into
Open
Conversation
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
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.
Fixes #24
Summary
PI_RULES_DISABLED,PI_RULES_MAX_RULE_CHARSandPI_RULES_MAX_RESULT_CHARSare documented in five places, butgrep -rn "process\.env" src/returned zero matches —src/index.tsbuilt its config fromdefaultConfig()unconditionally, so all three were inert. The engine already honored custom values; only the resolution step was missing.src/config.tsexportsconfigFromEnvironment(), followingcodex-rules'src/config.ts(samePiRulesConfigshape, sameisTruthyset). Two deliberate deviations:firstEnvcollapsed toreadEnv(pi-rules has no alias names to fall back to), andparsePositiveIntegervalidates the whole string instead of using bareNumber.parseInt, so"50abc"/"1.5"/"1e3"fall back to the default rather than silently becoming50/1/1. That keeps the promise in the new JSDoc and in Documented PI_RULES_DISABLED / PI_RULES_MAX_RULE_CHARS / PI_RULES_MAX_RESULT_CHARS env vars are never read #24 that a typo degrades to the documented default; since these variables were previously inert there is no back-compat cost.syncConfigFromFlags()now composes the flag with the env baseline (disabled || envDisabled) instead of overwriting it. pi applies boolean extension flags as presence-only (agent-session-services.js:if (flag.type === "boolean") flagValues.set(name, true)), sogetFlag("pi-rules-disabled")returns the registered defaultfalsewhenever the flag was not passed — resolving env only at construction time would have been silently clobbered on the very first hook.src/index.tsnow reads the realprocess.env, a vitest setup file clears the three variables so the pre-existing suites stay hermetic when a contributor has them exported in their shell (verified: without it,PI_RULES_DISABLED=1 npm testbroke unrelated tests).codex-rulesalso readsPI_RULES_MODEandPI_RULES_ENABLED_SOURCES; those are undocumented here, so I left them out rather than smuggle in new features — happy to follow up if you want full parity.README.mdneeds no wording change: "If1, disables injection" stays true (the accepted truthy set1/true/yes/onis a superset), and the documented12000/40000defaults are unchanged.RED-first evidence
test/env-config.test.tsbefore thesrc/index.tschange (3 failed | 3 passed):test/config.test.tsbeforesrc/config.tsexisted:Verification
npm run check(typecheck + biome) — exit 0 (only the pre-existing biome deprecated-config info)npm test(unit tests) — 268 passed / 17 files (252 pre-existing + 16 new, 0 skipped)npm run test:integration(integration tests) — 46 passed / 4 filesnpm pack --dry-run(release sanity) — 24 files,src/config.tsincludedpi -e ./src/index.tssmoke-tested locally, if behavior changedThe CI matrix is currently waiting on maintainer approval (first-time fork contributor), so I reproduced it locally:
npm ci+ all four commands on Node 22.21.0 and 24.17.0 — check PASS, 268 unit, 46 integration, pack PASS on both.End-to-end against the real
pibinaryUnit tests alone cannot prove the wiring reaches a real session, so I drove the actual
piCLI once per permutation with pi-rules loaded (pi -e ./src/index.ts -e <mock-provider> --provider mocklocal --model mock-1 -p "hello"), pointing it at a local OpenAI-compatible endpoint that records the system prompt pi really sends. Fixture:.omo/rules/big.mdwithalwaysApply: trueand a 5,000-char body, isolatedHOME.MAX_RULE_CHARS=50MAX_RESULT_CHARS=200DISABLED=1MAX_RULE_CHARS=abcMAX_RESULT_CHARS=-5DISABLED=0--pi-rules-disabled(flag only)DISABLED=trueDISABLED=yesMAX_RULE_CHARS=100DISABLED=1+--pi-rules-disabledMAX_RESULT_CHARS=300(no per-rule cap)MAX_RULE_CHARS=50abcMAX_RULE_CHARS=1.5MAX_RESULT_CHARS=1e3Cases 09 and 11 show each cap taking effect on its own; 12–14 show the stricter parser refusing partially-numeric input instead of silently truncating to 50 / 1 / 1.
Verified in the same live run: the extension loads in the TUI (
[Extensions] src) and/rules statusanswerspi-rules: 2 rules from 2 sources.Mutation checks on the new tests
To confirm the tests actually pin the wiring rather than passing incidentally, I removed one line at a time and re-ran them:
src/config.tsmaxResultCharsresolutionexpected 5128 to be less than or equal to 223maxRuleCharsresolutionexpected … to have a length of 50 but got 5000expected 1 to be 12000Rule-loading impact
[Unreleased]if user-facingAtomic commits
de1cd02fix(config): resolve documented PI_RULES_* env vars— newsrc/config.ts+ unit tests, no consumer yet17e566cfix(config): apply env config at startup and keep disabled across flag sync— wiring + the flag-composition fix, with its integration tests5562cd1docs: align changelog and jsdoc with env var support— CHANGELOG entry;PiRulesConfigJSDoc no longer claimspackage.jsonresolution, which does not exist983c05efix(config): reject malformed numeric env values— full-string integer validation2d1e52btest(config): cover per-rule and per-result caps separately— the earlier single test proved only the per-rule cap and its bound depended on the absolute rule patha525477test: isolate suites from ambient PI_RULES_* env— vitest setup file so existing suites stay hermeticOne thing I noticed but did not touch
While writing the
tool_resulttest I found thattest/extension-registration.test.ts's#given disabled true #when tool_result emittedand#given mode="static" #when tool_result emittedcurrently assert vacuously:project.write()returns the symlinked/var/folders/…path whilecwdisrealpathSync.native'd to/private/var/folders/…, soextractToolPaths→findProjectRootnever matches and the handler returnsundefinedregardless of the flag. My own test wraps the target inrealpathSync.native()so it exercises the real path. Left the existing tests alone since it is out of scope here — happy to send a separate PR if you want them tightened.