Repository navigation
fix(config): accept settings-only machine files - #1468
huong8373tt-beep wants to merge 4 commits into
Conversation
No other confident findings in the specified diff. |
No other confident findings in the specified diff. No PR code was run, built, or installed. |
|
Addressed the requested real-CLI verification on the built dist/index.js. I created an isolated temporary HOME and local bare Git remote, pre-seeded ~/.teamai/config.yaml with a settings-only file (no repo) containing: ai:
tool: codex
recallEnabled: true
workflows:
tools:
default: codexThen ran: teamai init <synthetic-git-url> --provider git --scope user --agent claudeThe command exited 0 and reported teamai initialized successfully!. The resulting config contained the new user install record (repo, scope: user) and retained all pre-existing settings above. The explicit checks for repo_key, scope_user, ai_preserved, recall_preserved, workflow_tool_preserved, and no_config_error all returned true. I also pushed ae3d8bb, which makes the existing symlink-only fixture return cleanly when Windows does not grant symlink creation permission; its functional assertion still runs wherever symlinks are available. The focused config suite now passes locally: 22 passed. |
No other confident findings in the specified 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.
@huong8373tt-beep Thanks, this is step 1 of #1459, and the main part works. We ran it with the built CLI against main, with a settings-only ~/.teamai/config.yaml (no repo) beside a workspace-only or self-only install:
- No more config errors.
status,doctor,pullandworkflow cancelprint no config error; onmainthey print 7, 29, 160 and 1 lines. - Valid settings carry over.
init --scope userover that file keeps every valid setting, asks nothing and sets nothing aside. - Broken installs still fail. A file that fails validation, or has
repobut nousername, still reportscould not be read. - Mixed releases. 0.28.0 reads the promoted install.
Three changes before this is approved:
- [P1 blocking]
init --scope usercan write an install it can't read. With a machine file holding onlyrecallEnabled: "yes"(a bareyesparses the same way),init --scope userexits 0. The nextstatusthen exits 1 withInvalid local config: recallEnabled: Expected boolean, received string, and the configcould not be read.saveLocalConfigcopies the file's keys into the new config without validating them; onmain,initrefused and left the file alone. #1459 gives the rule for a bad machine value: skip it, so the next level decides, and warn once. Please keep only the keys that validate, warn about each one dropped, and add a test with a wrong-typed value. - [P1 blocking] Real-CLI record. This changes runtime behaviour, so the body needs one real-CLI run after
npm run build(AGENTS.md, "Testing before a PR"). For example: a workspace install with a settings-only machine file,statusandpullwith no config error, theninit --scope userkeeping the settings. - [P2 non-blocking, needed for approval]
Closes #1459should beRefs #1459. This is step 1 of the four in #1459's suggested order. Merging as is would close the issue with steps 2–4 still open.
Non-blocking:
- [P2 non-blocking] Every key of the file is carried into the new install, not only member settings, so hand-written install keys such as
projectRootorexcludedSkillspass through. Carrying only the member-setting keys would match #1459. - [P2 non-blocking] The kept settings are applied after
init's first hook setup, so a machinecodexTrustEnabled: falsewould not hold during the firstinit --scope user. We found this by reading the code; we didn't run it. - [P3 nit] The two new commits make an existing Windows symlink test
returnonEPERM, so it passes without checking anything. Skipping it (ctx.skip()) would show it as skipped. It is also unrelated to this change, so it could go in its own PR. - [P3 nit] One
scope === 'user'guard is redundant, three comments still describe the oldai-only rule, andsaveLocalConfigrepeatswriteLocalConfig's body.
Overlap: #1465 conflicts by one line in serializeLocalConfig; keeping both changes resolves it.
CI review: we agree that the real-CLI record is missing. It missed the wrong-typed value above.
Verified at 00012c4; edbc860 only changes that test: tsc and lint pass. config-not-initialized.test.ts passes (22 tests), also under a symlinked TMPDIR, and its two new tests fail on main. Nine related e2e files pass. Not run here: the codexTrustEnabled case, init --http, and Windows.
|
Thanks for the detailed review. Addressed the blocking invalid-value case in commit
Validation after the fix:
The real-CLI settings-only |
No other confident findings in the specified diff. No PR code was run, built, or installed. |
|
Follow-up to the review:
PR2 and PR3 are now rebased on this PR’s updated head and have also been force-pushed. The dependent sequence is therefore:
For PR2, validation includes the focused overlay suite ( |
1 similar comment
|
Follow-up to the review:
PR2 and PR3 are now rebased on this PR’s updated head and have also been force-pushed. The dependent sequence is therefore:
For PR2, validation includes the focused overlay suite ( |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Re-review at 3ca7b95. The wrong-typed value fix works. One blocking item is left.
Blocking
- [P1 blocking] The real-CLI record is still not in the PR description. The body's Validation section still says the e2e fixture "will run in Linux CI". The record is only in the 16:29Z comment, which is prose with no output and predates 3ca7b95. Please paste the built-CLI run into the body, with the commands and their output, including the
recallEnabled: "yes"case. We ran it on our side and it behaves as described, so this is only the record.
Resolved since edbc860
| Finding | Evidence |
|---|---|
init --scope user with recallEnabled: "yes" wrote an install it could not read |
Built CLI: init exits 0 and warns Ignoring recallEnabled in …/config.yaml, the key is dropped, valid siblings (coAuthorEnabled, ai.tool, workflows.tools.default) stay, the next status exits 0. The new unit test fails against edbc860's src/config.ts. |
| Every key was carried, not only member settings | Only the 11 member-setting keys from #1459 are carried now. |
Closes vs Refs #1459 |
Refs kept. |
Non-blocking, fits this PR
- [P2 non-blocking] Promotion now drops non-member keys without a word. A machine file with
toolRoots,excludedSkillsorgitExcludeEnabledloses them oninit --scope user, with no warning. On maininitrefused and kept the file, and at edbc860 these keys were kept.toolRootsmatters most, because once the file is promoted it is the user scope and #1459 givestoolRootsthe rule "this scope, else this machine". Suggest one warning per dropped key, in the same form as the bad-value warning, or keepingtoolRoots. - [P3 nit] One bad entry drops the whole
workflowsblock.workflows: {tools: {default: codex}, max_parallel: "lots"}keeps nothing, so the validtools.defaultis lost. Per-entry handling fits #1473 too. - [P3 nit] The warning prints Zod's
(root):for a top-level value. #1459 suggestsrecallEnabled in ~/.teamai/config.yaml is "yes", not true or false, so it is ignored. ThesaveLocalConfigdocstring still says it retains all settings. - [P2 non-blocking] Still open from the last review: a kept
codexTrustEnabled: falseis applied afterinit's first hook setup. #1459 puts that in the overlay (#1473), so it can be closed there.
Verification at 3ca7b95
npm ci,npm run build,npx tsc --noEmit,npm run lintexit 0.config-not-initialized.test.ts: 23 passed, also with a symlinked TMPDIR.- e2e
init-ends-with-pull,init-agent-validation,user-uninstall-keeps-projects-1025,init-unattended: all pass. - Built CLI, isolated HOME and a local bare remote: a settings-only file next to a workspace-only and a self-only install (
status,pull,doctorwith 0 config errors),init --scope userover it, broken installs still rejected, a bad value, mixed keys, a badworkflowsentry. - With #1475 (fix for #1456) merged on top, a workspace pull over a settings-only machine file prints no config errors. #1475 alone prints
Invalid local configon that pull, so this PR closes the caveat #1475 mentions. - CI at 3ca7b95 is green. The red
E2E (fork-safe)entry belongs to the run cancelled when the PR was closed.
…e base of this stack
…ettings across workspace scopes (#1473) Steps 1 and 2 of #1459. A ~/.teamai/config.yaml that holds only member settings is read instead of rejected, and init --scope user promotes it keeping only valid member settings. Workspace and self configs now take the machine's member settings under their own, wherever the role assignment is applied, and never write a machine value back. A bad machine workflows.allow fails closed. Includes the commits of #1468. Refs #1459
Thanks, @huong8373tt-beep. This change landed with #1473 (292e2af), which carried these commits and the built-CLI record for the settings-only machine file, so I'm closing this PR. #1474 and #1477 can rebase onto main now. |
…e base of this stack
…ettings across workspace scopes (Tencent#1473) Steps 1 and 2 of Tencent#1459. A ~/.teamai/config.yaml that holds only member settings is read instead of rejected, and init --scope user promotes it keeping only valid member settings. Workspace and self configs now take the machine's member settings under their own, wherever the role assignment is applied, and never write a machine value back. A bad machine workflows.allow fails closed. Includes the commits of Tencent#1468. Refs Tencent#1459
Summary
Treat ~/.teamai/config.yaml without a repo key as a settings-only machine file rather than an initialized user installation. When init --scope user promotes that file, preserve valid member settings and skip invalid values with a warning.
This is step 1 of #1459, so this PR references the issue instead of closing it.
Validation
Built CLI verification recorded in review
The maintainer ran the built CLI with an isolated HOME and a local bare Git remote. The pre-seeded settings-only ~/.teamai/config.yaml was:
Commands exercised:
Observed results:
The related E2E fixture is skipped when Windows cannot create symlinks; its functional assertion runs where symlinks are available.
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.
Refs #1459