Repository navigation
Conversation
The PR description provides a representative real-CLI verification record; no missing-testing finding applies. Review limitation: The requested three-dot diff cannot resolve in this shallow checkout, and remote access failed. These findings come from the two WorkBuddy commits ending at the specified head; I cannot certify the complete requested diff. No PR code was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@kittimzhe Thanks. The branch at da90ceef conflicts with main in src/doctor.ts, src/resources/base.ts and src/types.ts, mostly because of the locate work from #1221. That resolution touches the same delivery and install-probe code this PR changes, so we'll review the rebased head rather than this one. Please rebase onto current main and rerun the real-CLI record on the result.
There was a problem hiding this comment.
Adding to the review above, after the rebase:
- The first P1 from the bot review (both roots present) is the reporter's own machine: the issue says
~/.workbuddy/is still there withlogs,settings.jsonandskills. So as written this PR does not fix the reported install. - The same check may also flip back on a clean 5.7 install. The probe now says WorkBuddy is installed, so if any surface still pointed at
.workbuddy/(user rules, agents, MCP) creates~/.workbuddy/on the first pull, the swap stops from the second pull on. I have not run that case. A user-scope record with a team that has rules would show it. - [P2 non-blocking]
Fixes #1273closes the issue at merge, while context injection on 5.7.6 still has no surface.Refs #1273keeps it open for the hooks work.
|
Both P1s taken, pushed:
Red-first: three new/changed tests — dual-root preference, swap surviving the default userScope merge, custom userScope redirect untouched. |
|
No new confident findings in the available WorkBuddy changes.
Review limitation: The requested three-dot diff fails with |
@kittimzhe Thanks, the two fixes in Two things before we review it again:
|
@kittimzhe Thanks for the update. |
c388d93 to
49ac887
Compare
…#1273) WorkBuddy 5.7+ moved its runtime to .workbuddy-ai/ and stops reading .workbuddy/, so a member on the new generation has only the new root and the install probe reported the tool as not installed — teamai integrated nothing, while every self-check stayed green. The probe now accepts either root as install proof, per generation list; which paths teamai writes for the new generation follows in the write-side change, blocked on the three questions in the issue (hooks file location, event vocabulary, old-format compatibility).
WorkBuddy 5.7+ scans skills from .workbuddy-ai/skills and stops reading .workbuddy/, so on a member whose install left only the new root, skills landed in a directory the app never looks at. scopedToolPaths now swaps the default skills path to .workbuddy-ai/skills when only the new root exists at the delivery base (user HOME or project root, by scope) — live-verified in the issue: 13 team skills delivered this way showed in the app's / menu on 5.7.6. Only the default path swaps, so a team that redirected skills on purpose keeps its own; other surfaces keep their configured paths (writes into the dead old layout are no-ops). Real-CLI record (workspace scope, .workbuddy-ai alone at the project root): pull --force delivers 'fixture-skill, teamai' into .workbuddy-ai/skills/, and no .workbuddy/ directory is created.
…-scope merge (Tencent#1273) Two findings from the PR review, both taken: - An upgraded 5.7+ install typically still has the leftover .workbuddy/ beside .workbuddy-ai/ (the reporter's machine is exactly that shape), so gating the swap on the old root's absence kept delivering skills into the directory the new runtime never reads. The new root now wins whenever it exists. - scopedToolPaths applied the swap before the user-scope merge, so a userScope.skills carrying the default .workbuddy/skills restored the unread old path over the adjusted one. The swap is re-applied after the merge (idempotent); a userScope redirect of its own is still never stomped. Real-CLI record, dual-root project root (.workbuddy + .workbuddy-ai): pull --force still delivers fixture-skill and teamai into .workbuddy-ai/skills/ and writes nothing into .workbuddy/skills.
Resolved: Both earlier findings are fixed: the new root wins when both remain, and the swap runs after user-scope overrides. Testing: The description documents a representative real-CLI verification; no missing-testing finding applies. Limitation: The requested three-dot diff fails with |
…eline (Tencent#1273) Main's Tencent#1221 moved skills delivery behind locate(), beside the probe re-homed into the own-root gate and the scopedToolPaths swap. locate now writes and probes .workbuddy-ai/ when the new root is the live generation: user scope may already carry the swapped default through scopedToolPaths, so both spellings count as the default. The constant moves to tools.ts, the tool registry, where base.ts's probe shares it. Real-CLI record rerun on the rebased head (workspace scope, only .workbuddy-ai/ at the project root): pull --force reports Synced 2 skills (2 new) — fixture-skill, teamai — both in .workbuddy-ai/skills/, no .workbuddy/ created.
49ac887 to
f427280
Compare
|
Rebased onto current main ( Real-CLI record rerun on the rebased head (same fixture: workspace scope, only Full suite on the rebased head matches main's baseline here — the four |
Resolved: All three earlier findings are fixed: the new root wins when both exist, default user-scope overrides retain the swap, and install detection no longer depends on the skills path. Testing: The description includes a representative real-CLI verification record; no missing-testing finding applies. Reviewed the complete requested three-dot diff successfully. No PR code was run, built, or installed. |
Refs #1273 (Phase 1 — the skills half; the hooks half needs the plugin-route design)
What
WorkBuddy 5.7+ moved its runtime to
.workbuddy-ai/and stops reading.workbuddy/, so on a 5.7+ member teamai reported the tool as not installed and delivered skills into a directory the app never scans — while every doctor self-check stayed green.Two changes:
.workbuddy(≤5.6) or.workbuddy-ai(5.7+) counts as installed, so a 5.7+ member is visible again..workbuddy-ai/exists at the delivery base (HOME in user scope, project root in project scope): the default.workbuddy/skillsbecomes.workbuddy-ai/skills. Only the default swaps — a team that redirected skills on purpose keeps its own path. Other surfaces keep their configured paths; writes into the dead old layout are no-ops the app ignores.Verification
Real-CLI record (workspace scope, only
.workbuddy-ai/at the project root):pull --forcereportsSynced 2 skills (2 new)—fixture-skill, teamai— and both land in.workbuddy-ai/skills/; no.workbuddy/directory is created. This mirrors @XiaoWanWH's live 5.7.6 result in the issue (13 team skills visible in the app's/menu after the same redirect).Nine red-first unit tests across both halves: old-root/new-root/both probe states, a bare
.codebuddynot counting, the swap firing only for the default path, not stomping team redirects, and project-root resolution.The hooks half (not here)
Context injection needs a plugin-route hooks writer (per the issue's evidence: no user-level
hooks.jsonis read on 5.7.6, only plugin-carried ones) — that design depends on the plugin mechanism and the event vocabulary captured in the issue, and I'll bring it as a follow-up once the direction is agreed.