Repository navigation
feat(workbuddy): recognize the 5.7+ .workbuddy-ai root and deliver skills there (#1273) - #1290
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
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
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 |
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. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@kittimzhe Thanks for the rebase. The skills half works with the built CLI. With only .workbuddy-ai/, WorkBuddy is detected as installed and skills land in .workbuddy-ai/skills in both user and project scope. A .workbuddy/-only member is unchanged, the new root wins when both exist, a redirected skills path is kept, and uninstall removes the skills from the new root. main has none of this yet. tsc, lint, the 18 new tests (8 fail on the base) and the related WorkBuddy unit and e2e files pass. One thing blocks.
- [P1 blocking] Opening the install gate makes every other WorkBuddy surface write into
.workbuddy/with doctor green. Reproduced in user scope.HOMEhas only.workbuddy-ai/, and the team has a skill, a rule and an MCP server.teamai init --scope user --agent workbuddy, thenteamai pull --force, exits 0. It printsSynced 1 rule(s)andUpdated teamai hooks in ~/.workbuddy/settings.json, and creates.workbuddy/rules/team-rule.md,.workbuddy/settings.jsonwith the Claude-format hooks, and.workbuddy/.mcp.json.doctorthen reports ✔ forteamai hooks in workbuddy settings,workbuddy rules are where the last pull wrote them,Rules delivered to workbuddyandMCP servers delivered to workbuddy. On the base, the same member gets nothing written and✖ workbuddy is installed. That is the false green #1273 reports, and this PR now produces it for 5.7-only members. The body calls these writes "no-ops the app ignores", but only the skills location is confirmed on 5.7.6. Project scope does not show it. Either keep the old-root writers (rules, MCP, hooks, agents) off for a member whose only root is.workbuddy-ai/, so only skills follow the new root, or have doctor stop reporting those surfaces as delivered there. - [P2 non-blocking] The issue comment that scoped the first PR asked for the bundled runtime to move with the probe.
src/bundled-runtime.tsstill looks for PortableGit and Node only under.workbuddy/binaries/and.workbuddy/bundled/node/. Please add it here or say in the body that it is a follow-up. - [P2 non-blocking] After a 5.6 to 5.7 switch, the old
.workbuddy/skillscopies stay, through the next pull and through uninstall. The pull notice tells the user to delete them, which is acceptable for now. - [P2 non-blocking] The new-generation rule is spelled out in four places, plus a one-entry
NEW_GENERATION_INSTALL_ROOTSmap. One helper that takes the host would keep them from drifting. - [P3 nit] The body says nine red-first tests; 8 of the 18 new tests fail on the base. Some test names cite review labels ("#1290 P1"), which will not mean anything once this merges.
Not run: WorkBuddy 5.7.6 itself, so whether it reads .workbuddy/rules or .workbuddy/.mcp.json is unverified; Windows.
The CI review's two [P1 blocking] findings on earlier heads are resolved here; we confirmed both with the built CLI. Its remaining [P2 non-blocking] note (the types.ts probe uses fs.existsSync and ignores the host) is right about the layering, but no CLI path makes it behave differently, so we rate it a nit. All CI jobs pass; the GitHub-provider e2e job is skipped by the fork policy.
|
Both P1s are already resolved in the rebased head — I believe the review saw a stale diff (its own limitation notes the three-dot diff could not resolve in the shallow checkout, from only two of the WorkBuddy commits; the reported lines On @SaulMoro's wider observation — agreed, and it is the next change I'll bring: the relaxed probe currently activates every surface on a 5.7-only member, but only skills have verified 5.7+ path evidence (the 13-skill |
…#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.
…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.
@kittimzhe Agreed on the two CI-review P1s: our review already lists both as resolved at f427280, checked with the built CLI (both roots present, and the user-scope override keeping the swap). The one blocking point from us is the wider one, and your plan covers it. Keep the skills swap, and let rules, MCP, hooks and agents keep requiring |
…encent#1290) The relaxed install probe activated every surface on a WorkBuddy 5.7-only member, but only skills have verified 5.7+ delivery evidence (the 13-skill `/` menu check in Tencent#1273) — rules, MCP, agents, blocks and hooks were being written into the dead `.workbuddy/` layout with doctor reporting them delivered. Scope the new root to the surface that earned it: `installedGateOf` now takes the resource, and only `locateSkills` passes `'skills'`, so every other locate surface keeps requiring `.workbuddy/` and skips a 5.7-only member until its real 5.7+ path is confirmed. `isToolInstalledForConfig` goes back to the strict root (its consumers are the non-skills surfaces: doctor's enabled-tool probe, uninstall's shared-path protection, known-agents), and `NEW_GENERATION_INSTALL_ROOTS` retires with it. A pull whose rules found no writable target now says so instead of counting rules no tool took (`Held N rule(s): no installed tool reads rules in this scope`) — the same reported-success-received-nothing the codebase's own doctor comment warns about. Verification: full suite 16388 passed, 6 failures baseline-verified on current main (git-discovery, submodule-checkout x2, workflow-workspace, gh-deadline, and the new workflow-place one from Tencent#1381; the lease-timeout flake passes isolated). Includes the reviewer-requested test: a team rule and an MCP server on a `.workbuddy-ai`-only home (user scope, the full default entry) find no writable target, so nothing creates `.workbuddy/`, while skills still take the new root. Real CLI matches: 5.7-only project with a team rule and an MCP server — skills synced to `.workbuddy-ai/skills`, rules held with nothing written, no `.workbuddy/` or MCP file created; old root present: rules delivered to `.codebuddy/rules` with frontmatter.
f427280 to
651693b
Compare
|
Pushed at The bundled-runtime note (PortableGit and Node under |
Resolved: The three earlier blockers are fixed: new-root preference, swapping after user-scope overrides, and install detection independent of custom skills paths. Testing: The description includes a representative real-CLI verification record; no missing-testing finding applies. Limitation: The requested three-dot diff fails with |
@kittimzhe Thanks, the plan works. On a member with only
Agreed that the bundled runtime is a separate contract: |
Review follow-up: `writableRulesTools` also required a rules directory, so a pull whose only rules target was a single file — Codex's user-scope `AGENTS.md` — printed 'Held N rule(s): no installed tool reads rules in this scope' while writing the rule, a regression against main in this PR's own output. Only `writable` decides now; directory and single-file targets report alike. Also splits the reviewer-requested test's loop into two explicit `locate` calls: `writable()` narrows by resource, so a union-typed `Location<'mcp' | 'rules'>` did not pass tsc (CI's Lint & Test caught what `npm run lint`, which skips test files, could not). Verified end to end on both gates: user scope with codex installed and one team rule prints 'Synced 1 rule(s)' and lands the rule in `~/.codex/AGENTS.md`; the 5.7-only workbuddy member still prints 'Held 1 rule(s)' with nothing written and no `.workbuddy/` created.
|
Both P1s are fixed at
The body is updated: the "no-ops the app ignores" sentence is dropped (it was not verified for the both-roots setup, where the old layout is still live), the test list reflects the current set, and the bundled runtime is stated as a follow-up. The skipped-surface hint I'd also leave for a follow-up — it touches the mcp/agents/hooks branches of pull and this PR is already carrying the probe semantics. |
Resolved: All three earlier blockers are fixed: new-root preference, swapping after user-scope overrides, and install detection independent of custom skills paths. The inline-rule reporting finding is also fixed: Testing: The description documents a representative real-CLI verification; no missing-testing finding applies. Limitation: The requested three-dot diff fails with |
@kittimzhe Both blockers are fixed at 268c782. I need to revisit one point. Last round I rated it non-blocking, and this round's run changes that:
The bundled runtime and the leftover |
…et (Tencent#1273) On a member whose only WorkBuddy root is .workbuddy-ai/, skills arrive there, while rules, team instructions, agents, MCP servers and hooks still need .workbuddy/, so pull writes none of them. `workbuddy is installed` passes on the skills root and the delivery checks skip the gated surfaces, so doctor reported "All checks passed!". Doctor now fails one check per tool and missing root, naming the held surfaces and the root they wait for. The post-pull pass prints the same row after every pull.
…encent#1290) Pin that a single-file rules target such as Codex's AGENTS.md counts as writable, so pull does not report "Held" for rules it wrote, and that a .workbuddy-ai-only member has no rules target. Correct the applyWorkBuddyGeneration comment, which still called writes under .workbuddy/ no-ops, move the helper above scopedToolPaths' own docblock, and drop review labels from test names.
Resolved: All three earlier blockers are fixed: new-root preference, swapping after user-scope overrides, and install detection independent of custom skills paths. Inline-rule reporting is also fixed: Testing: The PR description documents a representative real-CLI verification; no missing-testing finding applies. Reviewed the complete requested three-dot diff successfully. No PR code was run, built, or installed. |
…ped (Tencent#1273) The hooks row was added only to a root another held surface had already created. A team that maps WorkBuddy to skills and settings alone left the map empty, so on a member with only `.workbuddy-ai/` doctor passed while no hook was written. Record every missing root the other surfaces wait for, configured or not, and hold hooks when neither of the hook writer's install roots exists.
…encent#1290) In a workspace, Codex, Pi, Muse, DSH and ZCode take rules through teamai's session hook or extension, which writes no rules file, so pull printed "Held ... no installed tool reads rules" while the rules were delivered. A run-time rules channel whose gates are met now counts as a receiver. The helper is renamed to rulesReceivingTools to match.
…ent#1273) Skills move to `.workbuddy-ai/skills`; every other surface waits for `.workbuddy/`, and doctor names the held ones. The troubleshooting note no longer sends that member to register hooks by hand.
Resolved: New-root preference, swapping after user-scope overrides, install detection independent of custom skills paths, and inline-rule reporting are fixed. No other confident findings. Testing: The description includes a representative real-CLI verification record; no missing-testing finding applies. Its older commit reference is not blocking. Reviewed the complete requested three-dot diff. No PR code was run, built, or installed. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
Thanks @kittimzhe. I pushed the remaining fixes on top: doctor now names the surfaces a .workbuddy-ai-only member does not receive (hooks included, even when the team maps nothing else), workspace rules delivered through a session hook or extension count as synced again as on main, and the sharing, admin and troubleshooting docs say what the new root gets. writableRulesTools is now rulesReceivingTools with its own tests. CI is green at 495daec. The bundled runtime and the leftover .workbuddy/skills copies stay as follow-ups.
…) (#1404) * feat(sessions): resolve each tool's session store through locate (#1221) Each TOOLS entry now declares its session platforms, and each platform declares its store and resume command in SESSION_PLATFORM_FACTS. locate(tool, 'sessions') and the session adapters resolve the store in one place, sessionStoreDir, so a store follows the tool's recorded root or home. CodeBuddy sessions now follow a moved home (CODEBUDDY_CONFIG_DIR, or a recorded one) instead of always ~/.codebuddy. * fix(sessions): locate and the adapters put a default session store under the same profile home (#1221) * refactor(sessions): drop the adapters' uncalled isAvailable and getDefaultStoragePath statics (#1221) * test(locate): give the WorkBuddy test host a profile home (#1221) #1290 added an in-memory Host before Host gained profileHome.
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/. Before this change, teamai treated a member with only.workbuddy-ai/as not having WorkBuddy installed. On an upgraded member, where both roots exist, it delivered skills into.workbuddy/skills, which the app no longer scans, and doctor stayed green.Changes:
.workbuddy(≤5.6) or.workbuddy-ai(5.7+) counts as installed for skills. Every other surface keeps requiring.workbuddy/..workbuddy-ai/exists at the delivery base (HOME in user scope, project root in project scope), including when an upgrade left.workbuddy/behind: the default.workbuddy/skillsbecomes.workbuddy-ai/skills. Only the default swaps — a team that redirected skills on purpose keeps its own path. Rules, team instructions, agents, MCP servers and hooks keep their.workbuddy/paths. With only.workbuddy-ai/they are held and nothing is written under.workbuddy/..workbuddy-ai-only member,doctorfailsworkbuddy receives rules, team instructions, agents, MCP servers, hooks, naming the.workbuddy/root they wait for, and exits 1. Before, it printed "All checks passed!" while pull delivered only skills. The post-pull pass prints the same row after each pull. The check comes from theinstalledgateslocatealready returns, so it fires for any tool whose skills arrive while another surface's root is missing. Hooks are listed when neither root the hook writer installs into exists, also for a team that maps WorkBuddy to skills and settings only. A member with.workbuddy/(alone or beside.workbuddy-ai/) gets no new row.Held N rule(s)only when no tool takes rules. A rules file pull can write counts, and so does a session hook, extension or plugin whose gates are met, so Codex, Pi, Muse, DSH and ZCode in a workspace still printSynced N rule(s), as onmain.docs/guide/sharing.md(both languages) andskill-data/setup/references/manage-admin.mdsay what a.workbuddy-ai-only member gets.skill-data/core/references/troubleshooting.mdno longer sends that member to register hooks by hand.Verification
Real-CLI record on 495daec (
npm run build, thennode dist/index.js, sandboxHOME, local bare team remote).Team with one skill, rule, agent, MCP server and team hook,
HOMEwith only.workbuddy-ai/,init <remote> --scope user --agent workbuddy --provider git --force:Team whose
toolPaths.workbuddyhas onlyskillsandsettings, sharing one skill and one hook, sameHOME:On b32608d the same run printed no
receivesrow, anddoctor --jsonexited 0 withok: true.Workspace scope, one team rule,
init <remote> --scope workspace --agent codex(and--agent pi), the tool's root in HOME and in the project:On b32608d both printed
ℹ [workspace] Held 1 rule(s): no installed tool reads rules in this scopewhile delivering the rule.mainprintsSynced 1 rule(s).Other runs on the same build:
mkdir ~/.workbuddy, thenpull --force):Synced 1 rule(s),Synced 1 agents, doctorAll checks passed!with noreceivesrow, the same asmain..workbuddy-ai/at the project root: skills land in<project>/.workbuddy-ai/skills, and doctor failsworkbuddy receives rules, team instructions, agents, MCP serversnaming<project>/.workbuddy. Hooks join the row when~/.workbuddyis missing too. With~/.workbuddypresent they are written there and are not listed.The earlier record from this branch still holds (workspace scope, only
.workbuddy-ai/:Synced 2 skills (2 new), no.workbuddy/created). It matches @XiaoWanWH's live 5.7.6 result in the issue (13 team skills visible in the app's/menu after the same redirect).Unit tests: probe states for old root, new root and both, a bare
.codebuddynot counting, the swap firing only for the default path, team redirects kept, project-root resolution, a.workbuddy-ai-only home where a team rule and an MCP server find no target, doctor failing the held-surface check on a.workbuddy-ai-only home (also with only skills and settings mapped, where it names hooks) and staying silent with the old root, andrulesReceivingTools(empty on a.workbuddy-ai-only member, counting a single-file target such as Codex'sAGENTS.md, and counting Codex's session hook and Pi's extension in a workspace). The hooks-only doctor test and the two workspace cases fail on b32608d.npx tsc --noEmit,npm run lint,npx vitest run(577 files, 16472 tests) andnpm run test:e2e -- doctor-cli doctor-delivery-cli enabled-agents-whitelist instruction-targets local-agent-rules-993pass. No tests deleted. Not run: WorkBuddy 5.7.6 itself, Windows, and providers other thangit.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.The bundled runtime (PortableGit and Node under
.workbuddy-ai/binaries/) is a separate contract from where teamai writes resources —src/bundled-runtime.tsuses neither the probe norlocate— and is tracked as a follow-up, not part of this PR. So is removing the.workbuddy/skillscopies a 5.6 install left behind after an upgrade.