Repository navigation
fix(deploy): gate pi-compaction on pi like the other ~/.pi/agent entries - #2334
Conversation
On a box without pi the entry created a settings.json holding only the compaction keys, and doctor's defaultModel pin failed against it. A guard now requires an entry to declare the requires every sibling in its directory declares, with zsh-functions-sh exempt because bash sources it.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func TestManifest_AnEntryRequiresWhatEverySiblingInItsDirectoryRequires(t *testing.T) { | ||
| raw, err := os.ReadFile(filepath.Join("../../..", ManifestRel)) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| m, err := ParseManifest(raw) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| for _, name := range missingSiblingRequires(m.Configs) { | ||
| if _, ok := requiresExempt[name]; !ok { | ||
| t.Errorf("%s omits the requires every other entry in its directory declares; add it, or exempt it in requiresExempt with the reason", name) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Vacuous guard
THEORETICAL. TestManifest_AnEntryRequiresWhatEverySiblingInItsDirectoryRequires iterates missingSiblingRequires(m.Configs) and only errors for names not in requiresExempt. Once this PR makes the manifest clean, that slice is empty and the loop body never executes, so the test makes no positive assertion about the shipped manifest. If ParseManifest ever yielded an empty Configs (field rename, parse regression, manifest renamed) the test would still pass green and the regression it guards would go unnoticed. The helper's logic is covered by TestMissingSiblingRequires, so only the manifest read is unguarded; adding an assertion that the parsed manifest is non-empty (or that pi-compaction is present with Requires == "pi") closes the gap.
| if len(got) != len(tc.want) || (len(got) == 1 && got[0] != tc.want[0]) { | ||
| t.Errorf("%s: got %v, want %v", tc.name, got, tc.want) | ||
| } |
There was a problem hiding this comment.
Weak assertion
THEORETICAL. The comparison len(got) != len(tc.want) || (len(got) == 1 && got[0] != tc.want[0]) checks element names only when exactly one entry is missing. A future table case with two or more missing entries is compared by length alone, so wrong names or duplicates would still pass. No current case triggers it, but the table is clearly meant to grow; an element-wise comparison (or sorting and reflect.DeepEqual) removes the trap.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
⚙️ Agent run details
|
…ct comparisons On a clean manifest the guard's loop never ran, so an empty read would have passed. It now asserts pi-compaction is declared with requires pi and that every exemption names a live entry. The helper sorts its result and the table compares it with slices.Equal, with a two-miss case.
Review triageReviewed by pr-agent (github-actions, COMMENTED review with two inline comments on 71df086). CodeRabbit did not review.
|
1 participant
Problem
pi-compactionwas the one~/.pi/agententry inai/deploy.jsonwithout"requires": "pi"(added in #1938;pi,pi-mcpandpi-nan-providerall have it). On a box without pi, the deploy created~/.pi/agent/settings.jsonholding only the compaction keys, anddotf doctorthen failed its pin:pin "pi-deployed-default-model": key "defaultModel" not present. Found by the from-zero job on macOS (#2013 X1, #2331). A box with pi is unaffected: the merge lands next to the keys pi already wrote.Change
ai/deploy.json:pi-compactiondeclares"requires": "pi".cli/internal/deploy/manifest_requires_test.go: reads the shipped manifest and fails for an entry with norequireswhen every other entry in its destination directory declares the same one. A directory mixing tools ({HOME}: tmux, zsh) imposes nothing.zsh-functions-shis exempt by name with its reason (bash sources it too).Evidence (macOS arm64)
Mutation: the
zsh-functions-shexemption removed -> the guard FAILs namingzsh-functions-sh, so the exemption is load-bearing.tests/verify-setup.batsalso referencesdeploy.json; it runs in theintegrationjob.Knowledge
requirescontract (BUG-058: the harness deploy never prunes, so records deleted from the repo keep failing doctor from the mirror #843, OPS-036: setup-linux.sh runs dotf tools install before Node.js exists, so the npm catalog tools (bw, opencode) cannot install on a clean box #1312)SDD skip rationale
One manifest line plus its guard test; no production Go changes.