fix(coding-agent): honor PI_RULES environment settings - #670
Conversation
code-yeongyu
left a comment
code-yeongyu
left a comment
There was a problem hiding this comment.
[sisyphus-bot] brutal review of PR #670
Reviewed 8 changed files at commit 7525d38.
Findings: 0 critical, 0 major, 4 minor.
The implementation is solid — the strict positive-integer parser correctly rejects all malformed inputs (empty, whitespace, zero, negative, decimal, scientific, hex, signed, non-ASCII digits, overflow). The formatWithinResultBudget loop correctly converges and handles the edge case where the envelope alone exceeds the budget. The disabled || envDisabled composition is correct for a one-way disable flag. Test coverage is strong for the happy path and malformed-value cases.
The 4 minor findings are: (1) silent malformed-value fallback with no diagnostic, (2) missing test for env-disabled + flag-false sticky composition, (3) missing multi-rule test for budget convergence, (4) re-truncation performance in the budget loop. None are blocking — the code is correct, these are hardening and coverage improvements.
See inline comments for details.
| function parsePositiveInteger(value: string | undefined): number | undefined { | ||
| if (value === undefined) return undefined; | ||
| const normalized = value.trim(); | ||
| if (!/^\d+$/.test(normalized)) return undefined; | ||
| const parsed = Number(normalized); | ||
| return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : undefined; | ||
| } |
There was a problem hiding this comment.
[sisyphus-bot] minor: parsePositiveInteger silently returns undefined (→ default) on malformed values with no diagnostic, log, or warning. A user who typos PI_RULES_MAX_RULE_CHARS=50abc silently gets 12000-char rules instead of 50. The RuleDiagnostic type (types.ts:137-140) exists with severity warning/error and could carry this. While the PR body documents 'defaults are kept on malformed values', the complete absence of any feedback makes troubleshooting unexpected behavior very difficult — the user has no way to know their env var was rejected.
Fix: Emit a console.warn (or capture malformed values in a diagnostics array on PiRulesConfig / SessionState) when parsePositiveInteger rejects a non-empty value, including the env var name and the rejected value.
| it("keeps the presence-only disabled flag working", async () => { | ||
| const runner = await createRunner(); | ||
| runner.setFlagValue("pi-rules-disabled", true); | ||
| expect(await emitStatic(runner)).toBe(""); | ||
| }); |
There was a problem hiding this comment.
[sisyphus-bot] minor: Missing test for env-disabled + flag-false sticky composition. The disabled || envDisabled composition in index.ts:50 means that when PI_RULES_DISABLED=1 is set, the runtime flag pi-rules-disabled=false CANNOT re-enable rules (false || true = true). This is designed per the changes.md, but no test verifies it. If someone refactors || to && or removes envDisabled, no test would catch the regression — the 'keeps the presence-only disabled flag working' test only tests flag-true with env unset.
Fix: Add a test: set PI_RULES_DISABLED=1, create a runner, set flag pi-rules-disabled to false, emit static, and assert the result is still empty (rules stay disabled).
| it("caps the complete dynamic tool-result block", async () => { | ||
| process.env.PI_RULES_MAX_RESULT_CHARS = "600"; | ||
| const block = await emitDynamic(); | ||
| expect(block.length).toBeLessThanOrEqual(600); | ||
| expect(block).toContain("Additional project instructions matched for src/index.ts:"); | ||
| }); |
There was a problem hiding this comment.
[sisyphus-bot] minor: Missing multi-rule test for formatWithinResultBudget convergence. All budget tests use a single static rule (AGENTS.md, 5000 chars) and a single dynamic rule. The re-render loop in formatter.ts:26-42 is only exercised with one rule, where convergence is trivial (one truncation step). With multiple rules, the loop may iterate multiple times as truncateBudget reduces different rules, and no test verifies the loop correctly handles the multi-rule case where reducing one rule's body isn't enough and the budget must be further reduced.
Fix: Add a test with 3+ static rules (different sizes) and a tight PI_RULES_MAX_RESULT_CHARS (e.g., 200) that forces the loop to iterate 2+ times. Assert block.length <= maxResultChars and that at least one rule is truncated.
| function formatWithinResultBudget( | ||
| rules: ReadonlyArray<LoadedRule>, | ||
| options: FormatOptions, | ||
| render: (truncatedRules: ReadonlyArray<TruncatedRule>) => string, | ||
| ): string { | ||
| let bodyBudget = options.maxResultChars; | ||
| while (bodyBudget > 0) { | ||
| const truncatedRules = truncateRules(rules, { ...options, maxResultChars: bodyBudget }); | ||
| if (truncatedRules.length === 0) return ""; | ||
|
|
||
| const block = render(truncatedRules); | ||
| const overflow = block.length - options.maxResultChars; | ||
| if (overflow <= 0) return block; | ||
| bodyBudget = Math.max(0, bodyBudget - overflow); | ||
| } | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
[sisyphus-bot] minor: formatWithinResultBudget re-truncates ALL rules from scratch on every iteration (truncateRules(rules, {...options, maxResultChars: bodyBudget})). When the overflow is small (e.g., 1 char over budget due to envelope overhead), the loop re-truncates every rule body and re-runs truncateBudget. For a large number of rules with a tight budget, this is O(n_rules * iterations). The loop terminates correctly (bodyBudget strictly decreases), but the performance is unnecessarily poor for the multi-rule case. Consider binary-searching bodyBudget or caching per-rule truncation and only trimming the last rule that exceeds the budget.
Fix: Binary-search bodyBudget between 0 and maxResultChars instead of linear decrement. Or: truncate each rule once to maxRuleChars, then iteratively trim the last rule by the overflow amount, avoiding full re-truncation.
1 participant
Summary
pi-rules#25environment configuration behavior intoSenpi's built-in rules extension
PI_RULES_DISABLED,PI_RULES_MAX_RULE_CHARS, andPI_RULES_MAX_RESULT_CHARSwith strict positive-integer parsing andpresence-only flag composition
budget so the complete static/dynamic block respects the configured limit
Upstream source: code-yeongyu/pi-rules#25
Upstream merge commit:
12ad906f0b29e949ebbd1f89d8f85789578aa6e6RED -> GREEN evidence
parseInt) failed exactly the malformed-value test;restored strict parser returned 8/8 GREEN
against a 600-character cap; restoring it returned GREEN
Validation
npx vitest --run test/rules-env-config.test.ts test/rules-before-agent-start.test.ts:11/11 passed
npm run check: passednpm run build: passed after syncing currentorigin/maintests failed from a pre-existing unhandled
No API key found for anthropicrejection. The implicated
agent-session-concurrent.test.tspassed 7/7 inisolation in the same built worktree; this change does not touch session,
model, provider, or auth code.
Real CLI QA
Driven through the real source CLI and local fake model server with isolated
HOME/session/config directories and a 5,000-character
AGENTS.md:PI_RULES_DISABLED=1PI_RULES_MAX_RULE_CHARS=50PI_RULES_MAX_RESULT_CHARS=30050/30050abc1.51e3Local evidence:
local-ignore/qa-evidence/20260803-pi-rules-env/Summary by cubic
Honors
PI_RULES_DISABLED,PI_RULES_MAX_RULE_CHARS, andPI_RULES_MAX_RESULT_CHARSin the built-in rules extension and fixes result-length budgeting so static and dynamic blocks respect configured limits. Backports behavior fromcode-yeongyu/pi-rulesPR#25.PI_RULES_DISABLEDaccepts1,true,yes,on(case-insensitive) and composes with the runtime flag instead of being overwritten.<project_rules>envelope, headers, and sentinels inmaxResultChars; re-render until the complete block fits, returning no envelope if none can fit. Applies to both static and dynamic blocks.rules-env-config.test.tsand cleared env in test setup.Written for commit 7525d38. Summary will update on new commits.