Repository navigation
fix(import): stop same-named --dir checkouts overwriting each other, and keep AI components on --skip-enrich - #1440
Conversation
|
Two heads-ups before either of us has to rebase. #1427 predates this PR and changes the same function. 1.
One of those two tests has to change once the order is decided. I have no attachment to mine: refusing is the stricter reading, and if this lands first I will rewrite that case. 2. Your note on where the origin-less comparison belongs is right, and the field is already in the file you read. 3. One correction to 4. |
|
Agreed on all four. Whichever lands second takes the conflict, and I'm fine with either order.
|
|
Thanks, that all lands. Three answers so the port does not hit a surprise. On (2) — the schema note is right, but it is yours to make, not mine. On (3) — I measured the move case on the built CLI before answering. Same checkout copied to another path under the same basename, then So in On (1) — agreed, the refusal is yours. If |
|
I independently ran this head (built CLI, Verified
Remaining split, not a blocker here Two origin-less checkouts with the same basename still replace each other, because there is no source identity to compare. That is the #1427 gap ( @SaulMoro @jeff-r2026 could one of you take the review? The only remaining gate is |
The PR description documents sufficient testing: a build followed by representative real-CLI runs, with outcomes and deferred coverage stated. No testing-record finding. No PR code was run, built, or installed during this review. |
d0323e8 to
6be073d
Compare
|
Fixed in 6be073d. On impact: nothing in |
|
No new findings in the specified PR diff.
No PR code was run, built, or installed during this review. |
6be073d to
cad9e39
Compare
|
#1427 landed first, so I rebased and ported its comparison in cad9e39.
|
|
No new findings in the specified PR changes.
The shallow checkout prevented the three-dot command, so I recovered the shared parent from commit metadata and excluded unrelated base changes. 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.
@hz89Dev Thanks, and for porting #1427's root hash. Checked with the built CLI at cad9e39 against main (37f6139):
/a/svcthen/b/svcwith different origins now givegithub__acme__svc/andgithub__other__svc/, and A's curated doc stays.mainkeeps onesvc/and loses it.svc/published bymainfrom the same repo moves once togithub__acme__svc/, with docs and_domains.json, and no file in the teamwiki still namesevidence/code/svc/. The next import runs incrementally, and the kept AI components point at the newdocPath.- An origin-less checkout over remote-published evidence, and a checkout whose origin was removed, exit 1 with the refusal and leave
svc/unchanged. --from-repo, an HTTPS clone and an SSH clone of the same repo land in one directory. Origin-less and subdirectory imports keep their old names.--skip-enrichkeeps the AI components (authstaysINFERREDwith its responsibilities) and still prunes a deleted one.maindrops them to the fallback.- The #1427 port holds: an origin-less checkout re-imported after a change runs incrementally and keeps its curated doc and
localRootHash.
One change before this is approved:
-
[P1 blocking] Old directory-named evidence with no readable source identity is left beside the new slug with no message.
src/import-codebase.ts:246-258movesevidence/code/<legacySlug>/only whenpublishedSourcereturnssame. Forunknownit says nothing. Two cases reach that branch:- A team upgrading from 0.27.0. 0.27.0 publishes
evidence/code/svc/with no per-codebasesource-manifest.json, and its root manifest has noprojectorrepoUrl. Importing the same checkout with anoriginon this head givesevidence/code: github__acme__svc svc. The curated docs stay only insvc/, androuter.mdandindex.mdlist both codebases.svc/is never updated again, so recall keeps serving its stale facts next to the current ones. - An origin-less import whose checkout later gets an
origin. The outcome is the same, andsvc/carries alocalRootHashsidecar.
unknowncan't prove it's the same repo, so moving it would be wrong. A warning is enough:} else if (legacy?.kind === 'unknown') { log.warn(`teamwiki/evidence/code/${legacySlug}/ names no repository, so it was left in place. ` + `If it is an older copy of teamwiki/evidence/code/${slug}/, move any curated docs you want to keep from it, then remove it.`); }
- A team upgrading from 0.27.0. 0.27.0 publishes
Non-blocking:
- [P2 non-blocking]
warnStaleWikiScopeswas inserted between the/** Parse --max-files: … */comment andparseMaxFiles(src/import.ts:83-84), so that comment now sits on the new function. Moving the new function above the comment fixes it. - [P3 nit]
docs/guide/distribution.md:186and its Chinese page still saywiki: [checkout] # codebase slug, evidence/code/checkout/. A checkout with an origin is nowgithub__<owner>__checkout, and the new warning tells teams to change exactly that entry. "the slugteamai importprinted asproject:" would matchskill-data/wiki/SKILL.md:306. - [P3 nit] For a checkout whose origin was removed, the "no origin remote" refusal suggests renaming the directory or removing the evidence. Adding the origin back keeps the curated docs, since the import then moves
svc/, so it could come first. - [P3 nit] The body marks this breaking, but
CHANGELOG.mdhas no[Unreleased]line for the slug change. - Two origin-less checkouts with the same name and different roots still replace each other, as the body says. That is the #1422 gap 2 repro when neither checkout has a remote, so the issue stays open for it.
CI review: we agree the docPath issue it raised is fixed and the testing record is sufficient. It did not consider the unidentified old evidence above.
Verified at cad9e39: build, tsc and lint pass; the 11 related unit files (169 tests, the 4 changed files also under a symlinked TMPDIR) and the 7 e2e files that call the changed functions (114 tests) pass; 11 of the changed tests fail on main. Not run here: the resources.wiki warning with a real team repo (the unit test covers it, and so does the body), ci extract-mr with the real CLI (it shares the slug function), and hosts other than github.com.
@jeff-r2026 This changes the team repo layout (evidence/code/<name>/ becomes <provider>__<owner>__<repo>/, with a move), so it is yours to approve once the item above is fixed.
Two --dir checkouts with the same basename shared one evidence directory, so importing /b/svc replaced the evidence of /a/svc. A checkout root whose origin names a repo now takes the slug --from-repo, --from-mr and ci extract-mr give that repo.
A checkout named after its directory (no origin, or a subdirectory) can still share a slug with another codebase. When the published source-manifest.json names a repository and the checkout being imported is a different one, or has no remote, stop with an actionable error instead of replacing that codebase's evidence.
…mote slug Before this change, --dir published a checkout with an origin under its directory name. Now that it takes the remote's slug, its next import moves that evidence, curated docs included, to the new slug instead of leaving a duplicate codebase in the teamwiki. When the remote slug already has evidence, the old copy is kept and a warning names it.
A daily static sync (import --skip-enrich --incremental) replaced the published AI _manifest.json with the static fallback, losing each component's category and responsibilities and the edges between them. For the same repository, keep those entries for every component still in the code, refresh their entrypoints, and give new components the static entry.
They now share one slug, so comparing scheme-sensitive repo identities refused an import that CI over HTTPS and a member's SSH clone both make.
…repo --dir repo/apps/api and --dir repo/libs/api both take the slug api. Compare the extracted directory as well as the repository before treating the published evidence as this checkout's.
A role or project that lists a checkout's directory-named slug under resources.wiki no longer matches once the evidence takes the remote's slug, and recall then shares that codebase with everyone. Warn with the entries to change.
A skipped-enrichment import that moves directory-named evidence to the remote slug kept each component's old docPath, so the manifest pointed at pages under the removed directory. Take the doc path from the fresh extraction, as the entrypoints already are.
…n guarding its evidence Tencent#1427 compares localRootHash in sameRepository, which this branch replaced with publishedSource. Parse the published and staged sources as a remote or a local root, and treat two origin-less checkouts as the same source when the hash and the directory match. A differing hash stays unknown, so a checkout moved to another path re-imports instead of being refused. An origin-less checkout over evidence a remote repo published is now refused rather than replacing it, as the other refusals are.
…tory Evidence published before the per-codebase source manifest, or before the checkout had an origin, cannot be shown to be this repo's, so it is not moved to the remote slug. It used to stay beside the new slug with no message, and recall kept serving its stale facts. Name it, so the team can move the curated docs and remove it.
… checkout A checkout whose origin was removed is refused over its own evidence. Adding the origin back keeps the curated docs, since the import then moves the directory-named evidence to the remote slug.
cad9e39 to
ad4842e
Compare
|
All five are addressed in ad4842e, rebased on current
Agreed on the last point: this PR only refs #1422, so the issue stays open for two origin-less checkouts with the same name and different roots. The new runs are in the PR body table. |
Resolved: The earlier Testing: The description documents sufficient build and representative real-CLI verification, including the current head. No testing-record finding. No PR code was run, built, or installed. |
… evidence A checkout of other/svc next to evidence/code/svc/ from acme/svc leaves that evidence in place, and a role scoping svc still means it. Telling the role to switch to github__other__svc pointed it at the wrong repo.
|
Fixed in d9eea78. |
|
No new findings in the specified PR diff.
No PR code was run, built, or installed during this review. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@hz89Dev Thanks, everything from our last comment is resolved. With the built CLI at d9eea78:
- Evidence published by 0.27.0, and an origin-less import whose checkout later got an
origin, are now left in place with a warning, and the curated doc stays insvc/. - For a checkout whose origin was removed, the refusal suggests adding it back first. Doing that and importing again moves
svc/togithub__acme__svc/and keeps the curated doc. - In team mode, with a role scoping
wiki: [svc]: importing another repo namedsvcgivesgithub__other__svc/and noresources.wikiwarning, which was the CI review's point on ad4842e. Importing the same repo movessvc/and tells the role to change its entry togithub__acme__svc. - The
--max-filescomment is back onparseMaxFiles, bothdistribution.mdpages show the new slug, andCHANGELOG.mdhas the breaking change.
No new findings. Two origin-less checkouts with the same name and different roots still replace each other, as the body says; #1422 stays open for that.
CI review: we agree its scope-warning point on ad4842e was real and is fixed here (the new test fails on the previous source), and that the current head has no new findings.
Verified at d9eea78: build, tsc and lint pass; the 11 related unit files (172 tests, the 4 changed files also under a symlinked TMPDIR) and 7 related e2e files (114 tests) pass. Some CI jobs were still running at the time of this comment. Not run here: hosts other than github.com and providers other than git.
@jeff-r2026 This is ready for your decision on the layout change, together with #1448.
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@hz89Dev Approving, on top of our comment at this head. We also ran one team through a release change, on one shared wiki: 0.28.0, this PR, 0.28.0, this PR, 0.28.0. No curated content is lost: docs/curated.md moves to github__acme__svc/ once and stays there. When a teammate still on 0.28.0 imports after the move, svc/ comes back and the codebase is listed twice in router.md and index.md until everyone upgrades. This PR's build reports that on every run, and it never moves or deletes anything again. Two runs of this PR's build in a row on a fresh wiki leave the same directories.
[P3 nit] While a teammate is still on an older release, the svc/ the warning calls "an older copy" is the one they keep rewriting, so removing it lasts only until their next import. The warning, or the CHANGELOG line, could say that teammates on an older release recreate it until they upgrade.
@jeff-r2026 Breaking for the next tag: import --dir publishes a checkout with an origin under <provider>__<owner>__<repo> and moves its old directory-named evidence there; see CHANGELOG.
…and keep AI components on --skip-enrich (Tencent#1440) * fix(import): name a --dir checkout with an origin after its remote repo Two --dir checkouts with the same basename shared one evidence directory, so importing /b/svc replaced the evidence of /a/svc. A checkout root whose origin names a repo now takes the slug --from-repo, --from-mr and ci extract-mr give that repo. * fix(import): refuse to replace evidence published from a different repo A checkout named after its directory (no origin, or a subdirectory) can still share a slug with another codebase. When the published source-manifest.json names a repository and the checkout being imported is a different one, or has no remote, stop with an actionable error instead of replacing that codebase's evidence. * fix(import): move a checkout's old directory-named evidence to its remote slug Before this change, --dir published a checkout with an origin under its directory name. Now that it takes the remote's slug, its next import moves that evidence, curated docs included, to the new slug instead of leaving a duplicate codebase in the teamwiki. When the remote slug already has evidence, the old copy is kept and a warning names it. * fix(import): keep AI-written components on a --skip-enrich import A daily static sync (import --skip-enrich --incremental) replaced the published AI _manifest.json with the static fallback, losing each component's category and responsibilities and the edges between them. For the same repository, keep those entries for every component still in the code, refresh their entrypoints, and give new components the static entry. * docs(knowledge): say which slug import --dir uses * fix(import): treat SSH and HTTPS clones of one repo as the same source They now share one slug, so comparing scheme-sensitive repo identities refused an import that CI over HTTPS and a member's SSH clone both make. * fix(import): refuse to replace a same-named subdirectory of the same repo --dir repo/apps/api and --dir repo/libs/api both take the slug api. Compare the extracted directory as well as the repository before treating the published evidence as this checkout's. * refactor(codebase): resolve a checkout's anchors once per slug lookup * fix(import): name roles and projects still scoping the old --dir slug A role or project that lists a checkout's directory-named slug under resources.wiki no longer matches once the evidence takes the remote's slug, and recall then shares that codebase with everyone. Warn with the entries to change. * docs: describe the --dir slug and refusal precisely, in the wiki skill too * fix(import): point kept AI components at the evidence they now live in A skipped-enrichment import that moves directory-named evidence to the remote slug kept each component's old docPath, so the manifest pointed at pages under the removed directory. Take the doc path from the fresh extraction, as the entrypoints already are. * fix(import): recognise an origin-less checkout by its hashed root when guarding its evidence Tencent#1427 compares localRootHash in sameRepository, which this branch replaced with publishedSource. Parse the published and staged sources as a remote or a local root, and treat two origin-less checkouts as the same source when the hash and the directory match. A differing hash stays unknown, so a checkout moved to another path re-imports instead of being refused. An origin-less checkout over evidence a remote repo published is now refused rather than replacing it, as the other refusals are. * fix(import): warn about directory-named evidence that names no repository Evidence published before the per-codebase source manifest, or before the checkout had an origin, cannot be shown to be this repo's, so it is not moved to the remote slug. It used to stay beside the new slug with no message, and recall kept serving its stale facts. Name it, so the team can move the curated docs and remove it. * style(import): keep the --max-files comment on parseMaxFiles * docs: scope a wiki entry by the slug teamai import prints * fix(import): suggest adding the origin back before renaming a refused checkout A checkout whose origin was removed is refused over its own evidence. Adding the origin back keeps the curated docs, since the import then moves the directory-named evidence to the remote slug. * docs(changelog): note the --dir slug change for Tencent#1422 * fix(import): suggest a new wiki scope only when the old name holds no evidence A checkout of other/svc next to evidence/code/svc/ from acme/svc leaves that evidence in place, and a role scoping svc still means it. Telling the role to switch to github__other__svc pointed it at the wrong repo.
Summary
teamai import --dir /a/svcfollowed by--dir /b/svcwrote both codebases toteamwiki/evidence/code/svc/, so the second import erased the first, curated docs included. A dailyimport --skip-enrich --incrementalalso replaced the AI-written_manifest.jsonwith the static fallback. Now a checkout with anorigingets the slug--from-repoandci extract-mralready use, an import never replaces evidence that names another source, and--skip-enrichkeeps the AI component entries.Type of Change
The
--dirslug for a checkout with anoriginchanges (svcbecomesgithub__acme__svc), and an import over another source's evidence now fails instead of replacing it.Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpasses: 16623 passed. 7 timeouts inworkflow-local-driver,workflow-reconciliationandworkflow-settle-cost, which this change does not touch, failed locally; those three files pass 153/153 when run on their own. CI is the check for those.New tests, each seen failing before its fix:
anchors.test.ts: two checkouts namedsvcwith different origins getgithub__acme__svcandgithub__other__svc. A subdirectory keeps its own name.import-codebase.test.ts:--skip-enrich, with thedocPathof the slug they are published underimport-dir.test.ts:import --dirnames the role and project whoseresources.wikistill lists the old slug, and leaves that entry alone while the old slug still holds another repo's evidence.Two existing tests now expect the refusal, because those imports used to replace another source's evidence:
#1427's other origin-less tests pass unchanged on
publishedSource.Real CLI.
npm run build, then the builtdist/cli.jswith an isolatedHOME,--output, local git fixtures and a stubclaudeonPATHfor AI enrichment. Same script against a build ofmain(703d1c9) and of this branch (d0323e8):--dir /a/svc(acme), then--dir /b/svc(other)svc/, A's curated file gonegithub__acme__svc/andgithub__other__svc/, A's file keptsvc/bymain, then--dir /a/svcsvc/Moved teamwiki/evidence/code/svc/ to teamwiki/evidence/code/github__acme__svc/, curated doc kept/c/svcoversvc/that acme/svc publishedrepoUrlbecomes undefinedteamwiki/evidence/code/svc/ holds the evidence of git@github.com:acme/svc.git, and this checkout has no origin remote. Not replacing it. …,repoUrlunchanged--skip-enrich --incrementalafter one changeauth:category: component,EXTRACTED, no responsibilitiesauth:category: service,INFERRED,responsibilities: ["Signs users in"]--dir repo/apps/api, then--dir repo/libs/api… holds the evidence of apps/api in git@github.com:acme/svc.git, and this checkout is libs/api in git@github.com:acme/svc.git. Not replacing it. …svc/bymain, then--skip-enrich(6be073d)Moved … svc/ to … github__acme__svc/,authstaysINFERREDwithdocPath: evidence/code/github__acme__svc/auth.md/a/svcre-imported after a change (cad9e39)svc/, curated doc kept/b/svc(cad9e39)svc/replaced, curated doc not carried|
svc/published bymainwith itssource-manifest.jsonremoved (as 0.27.0 writes it), then the checkout with an origin (ad4842e) | (not run on main) |github__acme__svc/besidesvc/, warningteamwiki/evidence/code/svc/ names no repository, so it was left in place. …||
svc/published by the origin-less checkout, then its origin added (ad4842e) | (not run on main) | same warning,svc/kept || Origin removed from a checkout whose repo published
svc/, then added back (ad4842e) | (not run on main) | exit 1,… has no origin remote. Not replacing it. If this checkout is that repository, add its origin remote back and import it again; …; after adding it back,Moved … svc/ to … github__acme__svc/|The first six rows were run again on ad4842e with identical output, apart from the new last sentence of the "no origin remote" refusal.
The
resources.wikiwarning needs a team repo with roles, so it is covered by theimport --dirtest only, not the real-CLI run. Providers other than local git fixtures and agents other than the stub were not run locally.Related Issues
Refs #1422: gap 2, and what #1417 left of gap 1. Gap 3 landed in #1427, gap 4 is #1425.
Notes for Reviewers
localRootHashcomparison moved fromsameRepositoryintopublishedSource.CodebaseSourceSchemanow parses a manifest as a remote (repoUrl) or a local root (localRootHash). Two origin-less checkouts are the same source when the hash and the directory match. A differing hash staysunknown: that covers a checkout moved to another path, so the import goes through and only the curated docs are left behind. A different origin-less checkout with the same name therefore still replaces the evidence; nothing distinguishes it from a moved one.manifestMatchesSourcestill compares scheme-sensitive identities, so switching a clone between HTTPS and SSH costs one full extraction before increments resume.importCodebase, so it also guards--from-repoandci extract-mr. Their slugs come from the URL, so it fires there only when two hosts share an owner/repo path.ci extract-mralready had bothsvc/andgithub__acme__svc/. The next--dirimport leavessvc/in place and warns, so curated docs there are not overwritten.