Repository navigation
Conversation
Review was read-only; no PR code was run, built, or installed. |
55a9537 to
f1aed7a
Compare
Resolved: The five earlier code findings—refresh aliasing, invalid machine-value reporting, alias merging, workflow defaults, and team source paths—are fixed in the current diff. Review was read-only; no PR code was run, built, or installed. |
@huong8373tt-beep #1473 landed on main (292e2af, with #1468's commits), and this branch now conflicts with main. Could you rebase it onto main? We will review it at the new head. While you are there: #1477, which builds on this branch, lists five cases where
|
f1aed7a to
2a6ba80
Compare
|
Rebased #1474 onto merged #1473 / current I also addressed the remaining P2 from the review: Validation on the rebased head:
The PR is now a small, current-main-only doctor provenance diff. |
Resolved: The earlier doctor findings concerning machine-value validation, merged aliases, workflow defaults, local team source paths, and team usage-report policy are fixed. The earlier configuration-refresh and settings-promotion findings concern changes absent from the specified diff; they are not re-raised. No additional confident code findings. Review was read-only; 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.
Reviewed at 2a6ba80. The rebase is clean and usageReportEnabled now follows the team usageReport policy. Four of the five cases from our earlier comment still reproduce with the built CLI, and the body has no real-CLI record.
Blocking
- [P1 blocking]
updatePolicyandautoCompactTokensname the wrong winner when the machine has a user install. In a workspace withautoCompactTokens: 50000on a machine whose user scope sets 100000,doctor --jsonreports 50000 from the workspace as the winner, but a workspacepullwrites the user scope's value (autoCompactWindow: 133000in~/.claude/settings.json). Since #1475, the user scope decides both when there is one (update.ts:691forupdatePolicy; read, not run, asteamai updateneeds the registry). - [P1 blocking] An empty AI tool shows as the winner.
TEAMAI_AI_TOOL=or an empty scopeai.toolis reported as the winner witheffective: "", while the AI call skips both and runs on the next level. - [P1 blocking] Env kill switches are ignored. With
TEAMAI_RECALL_DISABLED=1(and no other level turning recall off), the report shows recalltruefrom the machine file, whilerecall statussays disabled.TEAMAI_CONTRIBUTE_HINT_DISABLEDis ignored the same way, andTEAMAI_MR_HINT_DISABLEDgoes through the same code (read, not run). - [P1 blocking] Map entries are missing.
workflows.tools.implementerandworkflows.allow.envdo not appear, andmodelAliasesis one row sourced from the scope although its value includes the machine's aliases. - [P1 blocking] No real-CLI record in the body. This changes runtime output; please paste one representative
teamai doctor --jsonrun from the built CLI.
Suggested fix for the first three: take effective from the resolvers the runtime uses and keep the tables only for the levels list: isRecallEnabled(ctx.localConfig, team ?? {}), isContributeHintEnabled, isMrHintEnabled, isUsageReportEnabled; resolveEffectiveUpdatePolicy / resolveAutoCompactTokens on the deciding config (loadLocalConfig() ?? ctx.localConfig); memberTools(ctx.localConfig)[0] for ai.tool. A test that every key's effective equals its levels[0].value would catch the next one.
Non-blocking
- [P3 nit]
codexTrustEnabledandcoAuthorEnabledhave no default row, unlike the other keys.
Checks: npx tsc --noEmit and npm run lint exit 0; doctor.test.ts passes 70/70, also with a symlinked TMPDIR, and the new test fails on main. The types.test.ts failure in CI is on main too (the sessions key from #1478 is not classified yet), not this PR's.
#1477 builds on this report and renders it as text, so it should rebase onto this branch once these land.
Resolved: Earlier doctor findings concerning machine-value validation, inherited aliases, workflow defaults, local team source paths, and team usage-report policy are addressed. The configuration-refresh and settings-promotion changes are absent from the specified diff and are not re-raised. No additional confident code findings. Review was read-only; no PR code was run, built, or installed. |
@huong8373tt-beep Thanks for 75c98ee. This branch now conflicts with main in |
75c98ee to
556564f
Compare
Resolved: Earlier doctor findings concerning machine-value validation, inherited aliases, workflow defaults, local team source paths, and team usage-report policy are addressed. The configuration-refresh and settings-promotion changes are absent from the specified diff and are not re-raised. No additional confident code findings. Review was read-only; no PR code was run, built, or installed. |
Re-reviewed at 556564f. Thanks, most of the earlier findings are fixed. Checked with the built CLI, these now match what runs:
Blocking
Non-blocking
Checks: #1477 renders this report and now conflicts in |
@huong8373tt-beep One more for the same fix round, raised on #1477 by its CI review and by @hz89Dev, and confirmed in this PR's code at 556564f. |
|
Thanks — I confirmed both new P1s at |
556564f to
b18e080
Compare
|
Rebased onto current Addressed the two latest blocking findings:
Validation on the rebased head:
The full Windows I will add the representative built-CLI |
Resolved: Earlier doctor findings concerning machine-value validation, inherited aliases, workflow defaults, local team source paths, and team usage-report policy are addressed. The configuration-refresh and settings-promotion changes are absent from the specified diff and are not re-raised. No additional confident code findings. Review was read-only; no PR code was run, built, or installed. |
|
The PR description is now updated with the requested representative built-CLI record from the rebased head ( The latest bot comment's only P1 was the missing record; that requirement is now addressed in the body. CI remains in progress on the rebased head. |
Re-reviewed at b18e080. Thanks, everything from the last pass is fixed. Checked with the built CLI:
One small item left:
Non-blocking
Checks: build, For #1477: a level can now carry |
|
Addressed the remaining P1 at
Validation on
The representative built-CLI |
|
No confident findings in the specified diff. Resolved
Review was read-only; 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.
Approving at 0851b8e. The last item is fixed: source now skips a level marked used: false (levels.find((level) => level.used !== false)), with a test for a workspace-only autoCompactTokens that the user install ignores. The fallback to the workspace's team when the user install's team cannot load is gone, and the types.ts comment edit is reverted.
Across the rounds, checked with the built CLI:
- with a user install and a workspace on different teams,
updatePolicyandautoCompactTokensmatch whatpullapplies, and the workspace's own values show as unused levels; - an empty
ai.tooland the environment kill switches agree with runtime; - map entries (
modelAliases.*,workflows.tools.*,workflows.allow.env) get one row each, dotted names included; - the default rows are present.
The body has a real-CLI record.
Thanks for working through all of these.
|
Thanks, @SaulMoro, for the careful and detailed review. It helped catch several subtle provenance edge cases that the initial implementation missed. I appreciate the guidance and have addressed the latest feedback in the current head; I’ll keep the follow-up work scoped and verified separately. |
@huong8373tt-beep #1486 just landed and also changes |
0851b8e to
ac45014
Compare
|
Rebased onto the current |
|
No confident findings in the specified diff. Resolved
Review was read-only; no PR code was run, built, or installed. |
Summary
This is the third dependent PR for #1459, building on #1468 and #1473.
memberSettingsprovenance toteamai doctor --json.pull/update, including the deciding user install's team for machine-wideupdatePolicyandautoCompactTokens.Validation
npm run build— passed on rebased headb18e0807.npm run typecheck— passed.npm run lint— passed with 0 warnings and 0 errors.git diff --check upstream/main...HEAD— passed.Focused resolver regressions — 2 passed:
reports the same winners as runtime resolvers across user, environment, and workflow-map settings;uses the deciding user install team and exposes workspace machine-wide values as unused.Built CLI end-to-end (isolated HOME and workspace, user and workspace teams intentionally different):
The command exited 0 and confirmed that
doctorfollows the user-install team while retaining the workspace values as explicitly unused levels.Full Windows
npm testreached a terminal result:16,292 passed, 1,101 failed, 193 skipped. The failures are dominated by pre-existing Windows/POSIX fixture incompatibilities (for example slash normalization inworkspace-childrenandspawn sleep ENOENT); the doctor provenance cases and focused resolver regressions pass. Linux/macOS CI is running on the rebased head.Disclosure
Slai Agent Runtime assisted with investigation, implementation, and test execution. The account owner reviewed and authorized this contribution and remains responsible for the submission.
Depends on #1473 and continues #1459 step 3.