Repository navigation
fix(pi): deliver user context to active override files - #1340
Conversation
|
No findings in the specified PR diff. The description documents sufficient testing, including a representative real built-CLI pull/uninstall verification. Deferred provider and agent combinations are not blocking. Review was read-only; I did not run, build, or install PR code. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, approving. This closes the #1221 Pi override item.
We built this head, its merge-base 83f6d1db and main 54622c3a, and ran teamai pull with both ~/.pi/agent/AGENTS.md and AGENTS.override.md present.
- base and main: the culture, claudemd and recall blocks go to
AGENTS.md, and the override keeps only the personal text. - this head: the blocks move into the override after the personal text, and the old blocks are removed from
AGENTS.md.
Pi's own loader decides which file counts. In @earendil-works/pi-coding-agent 1.4.0 (dist/core/resource-loader.js, loadContextFileFromDir), the candidates are ["AGENTS.override.md", "AGENTS.md", …] and the first existing file wins, so on base Pi never sees the blocks. 0.85.0 behaves the same. The new unit and e2e tests fail on base and pass here. tsc, lint and CI pass.
Suggestions:
[P3 nit]Pi 0.74.0 and the deprecated@mariozechner/pi-coding-agent0.73.1 don't readAGENTS.override.md. A user on such a version who still keeps an override file would lose the blocks after this change. That seems unlikely, but a line in the fix text or the docs naming the Pi version would cover it.[P3 nit]The body's test record describes a combined snapshot of several fixes. The record for this diff is the e2e run, which passes 43/43 here. The Pilabelspecial case inside the genericuserRulesFilecould live with the Pi descriptor. The troubleshooting section repeats the team-culture row.
#1265 moves instruction targets behind locate and conflicts with this PR in src/instruction-targets.ts. This PR owns the override choice, and #1265 owns where the path is resolved. Whichever lands second carries the other's change.
CI review: it reported no findings and accepted the testing record. We agree. We first suspected a Pi version gap, but current Pi reads the override first, so it is only the nit above.
7b84296 to
7198da7
Compare
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
|
No findings in the specified PR diff. The description documents sufficient testing, including a representative built-CLI pull/uninstall verification. Additional provider and agent coverage is not blocking. The earlier review raised no findings, so none require resolution. Review was read-only; I did not run, build, or install PR code. |
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks for the rebase. Re-checking at 7198da7 turned up a problem I missed when I approved 7b84296. The doctor read check and Pi's read list were the same there.
[P1 blocking] With AGENTS.override.md present, doctor reports that Pi cannot read the file teamai now writes. The write moves to ~/.pi/agent/AGENTS.override.md, but Pi's read declaration (src/tools.ts:559) still lists only ~/.pi/agent/AGENTS.md. On the built CLI, with the override present, teamai pull exits 0 but prints:
⚠ Pull finished, but 2 check(s) failed:
⚠ ✖ pi reads the rules teamai writes
→ pi does not read …/home/.pi/agent/AGENTS.override.md. Point toolPaths.pi.rules in teamai.yaml at a place pi reads.
⚠ ✖ pi reads the instruction blocks teamai writes
teamai doctor exits 1 with the same two failures. On 6ebb31c both are clean. The suggested remedy points the user away from the file Pi actually reads, which undoes the fix. The new unit test checks paths and the e2e checks only the exit code, so CI cannot catch this. Declaring both files as reads in src/tools.ts:559 (override first, then AGENTS.md) makes doctor exit 0 and tsc pass in a scratch copy. It also turns 36 Pi rows red in the locate-blocks / locate-rules matrix, which pin Pi's reads to AGENTS.md, so those expectations need the same update.
- [P2 non-blocking]
skill-data/setup/references/uninstall.md:91still names only~/.pi/agent/AGENTS.md. - The earlier nits still apply: the
tool === 'pi'branch insrc/locate.ts, the repeated troubleshooting text, and the test record describing a combined snapshot.
The rest checks out at this head. Build, tsc and lint pass. The PR's unit files and its e2e file (43/43) pass, and the new tests fail on the base for the right reason. Pi's source confirms override-first precedence.
The CI review reports no findings. We disagree: the doctor failure is in this diff, and it ran no code. All CI jobs pass. The PR now also conflicts with main in skill-data/core/references/troubleshooting.md after #1336 landed, so it needs another rebase.
7198da7 to
35e07cd
Compare
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
|
No findings in the specified PR diff. The description documents sufficient testing, including 43 built-CLI instruction-target cases covering Pi override delivery, doctor, migration, personal-text preservation, and uninstall. Deferred provider and agent coverage is not blocking. The earlier reviews raised no findings, so none require resolution. Review was read-only; I did not run, build, or install PR code. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, approving at 35e07cd. The blocking finding is fixed. Pi's read list (src/tools.ts) now declares AGENTS.override.md ahead of AGENTS.md, and the locate matrix and the uninstall reference follow. With the built CLI and an override present, 7198da7 printed two failed checks on pull and doctor exited 1. This head puts the blocks in the override, and doctor exits 0 with every Pi check passing. On the base the blocks go to AGENTS.md, which Pi does not read. The e2e now asserts the doctor exit code, which closes the gap that let this through. Build, tsc, lint, the PR's unit files and instruction-targets (43/43) pass.
It now conflicts with main again in skill-data/core/references/troubleshooting.md, after #1343 landed. Keeping both sections resolves it, and I'll merge it once it's rebased and green.
- [P3 nit] The
tool === 'pi'branch still sits in the genericlocateBlocks(src/locate.ts). The Pi override rows inlocate-blocks/locate-rulescite a "Pi 0.99.2 run" that predates the override read. The troubleshooting section repeats the team-culture row, and the body's "Broader regression record" describes an older combined snapshot.
The CI review reports no findings at this head. We agree. At 7198da7 it reported none while doctor failed, and it ran no code. All CI jobs pass.
- Select an existing AGENTS.override.md beside the configured Pi user context file, matching Pi loader precedence. - Route team rules, instruction blocks, doctor and uninstall through the shared target; strip only managed blocks from the stale normal file. - Preserve personal content and retain the configured-file fallback when no override exists; update English/Chinese guidance and troubleshooting. - Verify 373 target/user-rules tests, a built-CLI pull/uninstall scenario, type checking and lint.
- 将 Pi 覆盖文件用例迁移到主分支的 locate 和 inlineRulesLocationOf 接口，验证旧文件清理与默认目标恢复。 - 删除已经移除的 instructionTargetFile 和 userRulesFile 测试调用，避免恢复过时架构。 - 266 个相关单测和 43 个真实 CLI 端到端用例通过，构建、类型检查和 lint 通过。
- 在 Pi 用户指令读取声明中按优先顺序列出 AGENTS.override.md 和 AGENTS.md，避免正确交付后 doctor 错报不可读。 - 同步默认目录和 PI_CODING_AGENT_DIR 的路径矩阵，以及卸载说明中的覆盖文件目标。 - 真实 CLI 回归在修改前复现 doctor 退出码 1；修复后 1062 项相关单测、43 项指令交付 E2E、构建、类型检查和 lint 全部通过。
- 修正定位矩阵中的 Pi 版本证据说明，与当前覆盖文件优先加载契约一致。 - 1351 项定向单测及 44 项指令目标 CLI 场景通过，类型、lint 与构建通过。
35e07cd to
52b3478
Compare
|
No findings in the specified PR diff ( The description documents sufficient testing, including 43 representative built-CLI instruction-target cases covering override delivery, doctor, migration, personal-text preservation, and uninstall. Deferred provider and agent coverage is not blocking. Earlier reviews raised no findings, so none require resolution. Review was read-only; I did not run, build, or install PR code. |
|
Updated and rebased onto main Verification: 1351 focused unit tests passed; types, lint and build passed. Post-build real-CLI Updated head: |
@nateEc Re-checked after the rebase. Since 35e07cd only seven test labels changed. With the built CLI on this head, Two non-blocking notes for later:
|
Pi loads AGENTS.override.md before AGENTS.md. Deliver TeamAI rules and instructions into an existing user override, retire only managed blocks from the previously configured target, and keep personal text in both files. The descriptor read list and doctor follow the same priority.
Review follow-up:
Rebase and retain both troubleshooting sections. Update the Pi override test evidence labels to the current loader investigation. Keep override-first resolution, the doctor read-list fix, and personal content preservation.
Validation on this branch, rebased onto
fc8a229e:npx tsc --noEmit,npm run lint,npm run build,git diff --check: passed.npx vitest run src/__tests__/locate-blocks.test.ts src/__tests__/locate-rules.test.ts src/__tests__/instruction-targets.test.ts src/__tests__/tools-channels.test.ts: 1351 passed.npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/instruction-targets.test.ts: 44 passed, using the real CLI and isolated local Git/HOME fixtures.Local host: macOS, Node 22.22.1, Apple Git 2.50.1. Native Windows, live vendor applications and additional provider combinations were not run locally and are left to CI. This record replaces the earlier combined-snapshot validation paragraphs.
Refs #1221; this PR addresses only its stated independent defect.