Repository navigation
Conversation
Signed-off-by: G-type <162748908+gtref@users.noreply.github.com>
Review limitation: The requested three-dot diff fails because the shallow checkout has no merge base, and network access is unavailable. I inspected the two PR commits’ diff, but cannot confirm it exactly matches the requested range. No additional confirmed code findings; the exact-range review remains incomplete. |
* feat(push): place all distributed resources with project and role namespaces (Tencent#1101) * fix(push): harden namespace routing and review (Tencent#1101) * fix(push): read manifests after pull and guard capture/restore paths Three CI failures rooted in the same family of pre-pull / path-confusion bugs introduced with the namespace placement work: 1. syncWorkflowDefinitions unconditionally resolved resource namespaces (reading projects.yaml and roles.yaml) from validateWorkflows, which runs BEFORE pushCore refreshes the clone. A stale manifest mapped new resources to the wrong namespace (Tencent#649 review). Now the manifest is only read when namespaced workflow candidates exist, wrapped in try/catch so an unreadable manifest degrades to on-disk namespaces. 2. resolveResourceNamespaces at the docs/wiki annotation site had no error handling: a manifest that exists but cannot be read produced an unhandled rejection instead of the graceful exitCode=2 stop every other manifest-error path uses. 3. The pre-pull snapshot capture was widened from env-only to every placeable type, but restorePendingEnvFiles writes to clone/<relativePath>. For project-local sources (tool-format agents, rules, skills) that wrote tool-format bytes over the clone's canonical files, so the next scan reported "cannot read or parse canonical agent YAML" and dropped rules entirely. Now only sources inside the team repo clone are captured — the ones resetToCleanMaster can destroy. * chore: trigger CI * chore: nudge CI trigger * fix(push): do not stage pre-placement paths as superseded files placeNewResources marked the pre-placement root path as supersedes when moving a new resource into a namespace. pushRepoBranch then staged that path with git add, but the root file was never in the clone, so git aborted with 'pathspec did not match any files'. supersedes is for pushItem retiring a tracked file during a format rename (agent .md to .yaml); a placement from the shared root to a namespace is not a rename. * fix: address review findings — binary copy, prompt namespaces, env placement, role learnings, wiki source, and P2s P1 fixes: - repo-resource: use fs.copyFile byte copy instead of UTF-8 text I/O to prevent binary doc corruption - workflow/cache: resolve prompt namespaces independently of workflow layout; support nested prompt paths - env: add status field to envPushItem so placement routes env/env.yaml to env/<ns>/env.yaml - roles: include learnings in role namespace resolution (Tencent#1101: 'Learnings should support --project and --role') - wiki: point handler at teamwiki/evidence/code/ (authoritative source per manifest-schema.ts:112) - push: filter LOCAL_TOOL_TYPES from pre-refresh scan to suppress spurious warnings P2 fixes: - push-namespaces: add rootSegmentsFor() and withNamespace(rootSegments) for wiki's multi-segment root - review-branch: fix nested path identity collision (use full sub-path as name); preserve handler-provided names - review-branch: skip undeclared-namespace check in manifest-free teams - --role: warn when not declared in manifest/roles.yaml - status: count models/claudemd/learnings/wiki/workflows/prompts/scorers - push: simplify pushRepoResourceItem signature to LocalConfig only - docs: update distribution.md and --role/--project help text - push: tighten pre-refresh snapshot to only capture clone-backed items * fix: snapshot key mismatch and manifest-error namespace fallback - push: track prePlacementPath so the snapshot delete finds the entry keyed by the root path after placement rewrites item.relativePath - workflow/cache: a manifest that cannot be read now resolves to an empty namespace set (conservative) instead of falling back to 'every namespace active', which cached inactive workflows as runnable - commands.md: regenerate snapshot for updated --role/--project help text * fix: address second re-review — reuse-path snapshot, env copy, manifest-free prompts, nested names, rule names P1 fixes from re-review of d77e8f0: - push: reuseRecordedDestinations saves prePlacementPath before rewriting relativePath so the snapshot delete finds the root-path entry - env: pushItem copies file whenever source != destination (not just self mode), so env/env.yaml -> env/<ns>/env.yaml actually writes the file - workflow/cache: manifest-free teams discover prompt namespaces from disk so prompts/<ns>/deploy.md resolves for prompts/deploy.md - review-branch: parseRelativePath uses basename (not full sub-path) so nested names like 'api/guide' don't fail isSafeNamespaceSegment - rules: scanTeamForPull extracts name and namespace separately, so rules/ops/foo.md -> name 'foo', ns 'ops' instead of name 'ops/foo' P3: - workflow/cache: update stale comment about manifest-error fallback * fix: revert scanTeamForPull change, fix rule names in review-branch instead The rules.ts scanTeamForPull change (extracting name/namespace from the path) broke pull and uninstall behavior — CI failures in uninstall.test.ts and workspace-children.test.ts showed extra rules/fe/*.md globs and rules/platform/ops.md delivery. Revert rules.ts to its original form and instead handle the name/namespace extraction in review-branch.ts scanResources, where rules items now go through parseRelativePath like other non-handler-authoritative types. * fix(review): entry identities for hooks, wiki authoritative root, per-segment doc names - review-branch scans wiki at teamwiki/evidence/code (where the handler and pull read it) instead of wiki/, and READ_ROOTS materializes the same root, so wiki pages no longer bypass branch review - hooks are reviewed by their entry ids (pull's hooks reader identity), not by file name, so co-active hooks/a/hooks.yaml and hooks/b/hooks.yaml with disjoint ids are not duplicates; unreadable or entry-free files keep a file identity so naming and leak checks still see them - docs and wiki keep their bundle-relative sub-path as the resource name (below the namespace when declared, below the root otherwise), and namingFindings validates each path segment on its own, so shared nested docs like docs/api/guide.md pass review while co-active docs/a/api/guide.md and docs/b/ui/guide.md stay two resources - leakFindings checks a file once even when several resources share it - update the Chinese distribution guide to match the push behavior change * fix(review): nested prompts are shared paths, docs identities keep extensions - prompts join docs and wiki as nested types: a first directory is a namespace only when a manifest declares it, so a workflow's prompt_file: prompts/topic/note.md stays a shared prompt instead of failing review as an undeclared namespace 'topic' - docs/wiki/prompt identities keep the file extension, so docs/a/api/guide.md and docs/b/api/guide.png are distinct resources and no longer report a false duplicate - move the undeclared-namespace assertion onto workflows, which still treats its first directory as a namespace * fix(push): place nested prompts, and remember where a repo-backed placement landed - prompts join docs and wiki as nested types on the push side too: a first directory is a namespace only when a role or project declares it, so a new prompts/topic/note.md is shared content (isAtSharedRoot) that a placement still moves to prompts/<ns>/topic/note.md, the path a workflow resolves at runtime. resolveResourceNamespaces now reports the declared-but-inactive prompts namespaces that decision needs - remember where push placed a repo-backed resource (docs, wiki, prompts, workflows, scorers, models, env, hooks, claudemd, learnings) in state.placements, keyed by the path the author still edits, and record it only once the placement's blob is on the default branch (the same proof placedRules uses, so a PR closed unmerged leaves nothing behind) - a self-mode scan now compares that author copy against the destination instead of reading it as brand new: unchanged means already delivered, an edit is reported as that destination's modification, so the next push --all --role <ns> no longer collides with the file it placed * fix(push): reuse a placement record in env scans, and expire one whose file was deleted - EnvHandler's self-mode scan reads state.placements too: an env file an earlier push placed under env/<ns>/ is compared against that destination, so the unchanged .teamai/env/env.yaml is not new again (and does not collide with the file it produced), and an edit is reported on the placed path - a repo-backed placement record is dropped when its destination was deleted since the last checkpoint, even if the path exists again: what is there now is somebody else's, and following the record would route this author's edits onto it. Same rule and same checkpoint as placedRules/placedAgents, which now stays while any record — including these — survives - share namespaceOfPlacedPath in push-namespaces so both scans read the namespace a record's destination carries the same way * fix(push,workflow): retire a placed env original, keep explicit prompt namespaces - EnvHandler.pushItem retires the clone-local original once a placement has copied it, as pushRepoResourceItem does: a leftover untracked env/env.yaml was rediscovered as new by the next push, which then aborted because the placed env/<ns>/env.yaml already existed. A self-mode source lives in the active .teamai tree, outside the clone, and is kept — that is the copy the author still edits - resolvePromptFiles resolves a prompt_file whose first segment is already an active namespace as written: prompts/ops/note.md no longer looks up prompts/<other>/ops/note.md, which loaded a different file whenever both existed * test(push): use .md scorer names in the placement samples A scorer is scorers/<name>.md (frontmatter plus a prompt), and review --branch fails any other file under scorers/ but README.md (Tencent#1085), so the sample items name .md files rather than .py. * fix(push): compare a placed copy with what its push delivered A placement record held only the destination path, so the self-mode scan compared the author's root copy with the placed file as it is now. Once a teammate changed that file, an untouched copy read as an edit and was pushed over their change, with no warning. Keep the delivered blob with the record and compare the author's copy with it: equal is already delivered, different is the author's edit, which goes to the placed file unless that file changed too, in which case the scan stops and names the file to edit. An edit that lands refreshes the record. A record holding only the path, as earlier builds wrote, is dropped. * fix(push): judge a placed env file against what its push delivered The self-mode env scan compared an unchanged .teamai/env copy with the placed file as it is now. After a teammate fixed env/ops/env.yaml on the team repo, the author's old copy read as an edit and the next push carried it over the fix. Use the same placed-copy check as the other repo-backed scans, which compares against the blob the push delivered. * fix(push): require --role to name a declared namespace --role was taken as a literal namespace on every axis. In a team with manifest/roles.yaml or manifest/projects.yaml, a namespace no role or project declares for a type is one pull delivers to nobody, yet push wrote rules/<ns>/, docs/<ns>/, workflows/<ns>/ and the rest there after a warning that checked roles.yaml only. Resolve --role per type against the namespaces both manifests declare for that type's axis, the way --project resolves against its project, and stop with exit 2 otherwise. A team with neither manifest still takes any namespace. The push-role unit fixtures now declare the namespaces their tests pass. * fix(contribute): use the learnings namespaces of the member's roles Roles now declare learnings namespaces, and pull delivers learnings/<ns>/ to the role's members, but contribute still resolved learnings from active projects only. A role-only member's contribution went to the shared root, and --namespace rejected the role's own namespace. Resolve the active learnings namespaces from roles and projects, in the order pull merges them, in the one function contribute, recall, import and `projects list` read. * fix(push): let a scan error before the refresh reach the pull failure path The pre-refresh scan swallowed a handler's error and treated that type as having no changes, so the clone was refreshed without knowing what the type held. Let the error propagate to the same catch the env scan uses. * fix(push): say to merge a conflicting edit into the placed file The conflict message read as if the author's edit should replace the team's change in the placed file. Say the edit is merged into that file, keeping the team's change, before the shared-root copy is deleted. * docs(changelog): note the --role namespace check * feat(namespaces): resolve inactive scorers namespaces like prompts Pull has no scorers reader yet (Tencent#1085). Resolve the scorers namespaces a member does not select, in the same shape prompts use, so that reader can withhold them. * feat(contribute): accept an empty --namespace as the shared root A member whose roles or project declare one learnings namespace contributed there by default, and --namespace only accepted declared namespaces, so that member could not write to the shared root. A member with several namespaces reaches it by default. --namespace "" now names the shared root for every member. * test(push): push only the rule where --role names a knowledge namespace Five placement-record cases pushed a rule, a skill and an agent with --role be-know, which the roles manifest declares only for knowledge. Since --role has to name a declared namespace for each type it places, that push stops with exit 2. The cases are about rule records, so they push the rule alone. * test(push): keep a teammate's change from a self-mode checkout behind main Single-repo push compares the member's .teamai/ with a fresh origin/<default>, so on a checkout behind it a file a teammate changed reads as the member's edit and the branch carries the old content back. Cover a file only main changed, a file both changed, a namespaced destination, a format change that deletes a file main changed, a tool-directory copy edited on top of main, a shallow clone, a checkout with no shared history, and a stop for a file the push does not send. A genuine edit of a stale .teamai/rules copy now stops instead of overwriting the teammate's update. * fix(push): leave out team files main changed under a self-mode checkout behind it For each item sourced from .teamai/, compare the member's file with the merge-base and the tip of the default branch, at its source, its destination and any file the push deletes. A file only the default branch changed is left out with a line saying git pull brings it in. A file both sides changed, a file the push would delete, or one that cannot be classified for lack of shared history stops the push with exit 2 before anything is written, naming the files and the remedy. The stop applies only to items going out. Tool-directory copies keep their own checks: the pre-push sync moves them to the default branch's version while .teamai/ stays behind. --------- Co-authored-by: Saul Moro <saul.moro.gomez@gmail.com>
SaulMoro
requested changes
Oct 23, 2026
SaulMoro
left a comment
SaulMoro
left a comment
Collaborator
There was a problem hiding this comment.
This was generated by AI during triage.
@gtref Thanks. This branch conflicts with main in src/team-push.ts. Main caused it: #1291 (97f1e7b4) rewrote the same reportUsageToTeam block. The update callback now always returns { files, message } with pushIfUnchanged: true, and it confirms vote deltas through onDelivered. The resolution decides what that function does after this refactor, so we'll review the rebased head instead of this one. Please rebase onto current main and keep #1291's retry behavior.
Signed-off-by: G-type <162748908+gtref@users.noreply.github.com>
The requested three-dot diff now succeeds, resolving the earlier review limitation. No PR code was run, built, or installed. |
Author
|
@SaulMoro The latest commit should have pulled all recent changes and fixed merge issues in the team-push.ts file. Though it may still contain some issues. |
Collaborator
@gtref Thanks for the rebase. The conflict is gone, but this head has a new blocker. The earlier request stays open for it.
Not blocking:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request refactors the unreachable clone-reset path in the function reportUsageToTeam as stated in issue #1259.
Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpassesRelated Issues
Closes #1259
Notes for Reviewers
Nothing of note