fix: refuse a publish that would append to another project's history - #127
Conversation
Slugs derive from the trailing path segment of source_repo, so acme/whisper and openai/whisper both produce "whisper". Publishing the second wrote it as v2 of the first. The entry's version history then spanned two unrelated codebases, and since buildIndexEntry reads only the newest version's metadata, index.yaml attributed every prior version to whichever repo published last. Nothing surfaced the collision at publish time or afterwards. publishEntry now reads the source_repo recorded on the newest version and fails before writing when it denotes a different repository. Comparison is normalized so that re-publishing one repository spelled another way is unaffected: scheme or none, git@host:path SCP syntax, a www. prefix, a trailing .git, trailing slashes, backslash separators, and casing all collapse to the same form. Only spellings that are unambiguously the same target collapse, since the caller treats "different" as fatal. Three details worth noting for review: - The check sits ahead of the content-hash branch. Identical spec bytes take the metadata-only path, which overwrote the other project's source_repo and headline in place, so guarding only the new-version path would have left the quieter half of the bug. - forceNewVersion does not bypass it. That option means "another version of this entry", not "overwrite a different project". - Unreadable or malformed recorded metadata skips the check. There is nothing to compare, and refusing on unknown would turn a corrupt v1 into an unpublishable entry. A genuine repository move is the one legitimate reason to change the recorded value, so allowSourceRepoChange opts out, exposed on the MCP surface as allow_source_repo_change. docs/library-format.md promised auto-suffixing (-2, -3) for this case. That was never implemented, and silent suffixing is its own surprise for an agent-driven tool, so the doc now describes the refusal and the override. Eight tests. Three fail without the guard: the two-project collision, the metadata-only path, and forceNewVersion. The rest pin the normalization equivalences, keep genuinely distinct repos distinct, and hold the false-positive and malformed-metadata paths open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review of #127 found the guard refusing a legitimate re-publish, which is the failure direction the guard was supposed to avoid. normalizeSourceRepo ran the SCP branch before stripping the scheme and never stripped a userinfo prefix, so four common spellings of one repository each reduced to something different: ssh://git@github.com/acme/tool.git -> git@github.com/acme/tool ssh://git@github.com:22/acme/tool.git -> git@github.com:22/acme/tool https://git@github.com/acme/tool -> git@github.com/acme/tool https://user:token@github.com/acme/tool -> user:token@github.com/acme/tool Publishing once from an HTTPS clone and later from an SSH clone of the same repo was therefore refused as a different project. Reordered: scheme off first, then userinfo, then the SCP branch, which now runs only when there was no scheme, since that is the only place a colon means host:path rather than a port. Default ports 22, 80 and 443 are dropped; any other port is kept, because two services on one host can differ by port alone. The SCP branch also now requires a dotted host, so a Windows drive letter is not read as host:path. The original tests missed all of this: every URL form they used had either no scheme or no credentials. Widened to nine more equivalent spellings, and the distinctness assertions now pin non-default ports, drive letters, and a same-host different-path pair. Two tests fail against the previous normalization. 351 tests, suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…not have Review follow-ups on the collision guard. Case was folded across the whole reference, including absolute POSIX paths. /srv/Repos/tool and /srv/repos/tool are two directories on a case-sensitive filesystem, so folding them let a genuine two-project collision through the guard silently — the exact failure it exists to catch. Case is still folded for hosts, for the repository paths the forges serve over them, and for Windows drive paths, all of which are case-insensitive. This matters more than it reads: Pi records the analyzed directory as source_repo, so local paths are the common shape on that surface, not a curiosity. Repeated separators now collapse, so github.com//acme//tool no longer reads as a different repository from github.com/acme/tool. A leading // is preserved, since on Windows that is a UNC share rather than /server/share. The refusal message told the reader to "pass an explicit, distinct slug" or "set allowSourceRepoChange". Neither is reachable from Pi: /codecarto-publish takes no arguments and the override is MCP-only. It now describes the two remedies and names where each one lives, so the message is honest on whichever surface raised it. Closing the gap on Pi is separate UX work. Four tests, all failing against the previous normalization: the extended equivalence set, the case rules in both directions, an end-to-end refusal of two local directories differing only in case, and a check that the message names both override spellings and no longer prescribes one the caller may not expose. 354 total, suite green. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LmPQsx1esS4uHDzVbbKVZg
|
Pushed a third commit ( Case folding was too broad. The description says scheme, credentials, SCP syntax, port, It is not a curiosity: Pi records the analyzed directory as Repeated separators now collapse. The refusal message named remedies the caller may not have. It said to "pass an explicit, distinct slug" or "set Four tests, each failing against Also filed the two other items the description parks in "Deliberately not in this PR", plus one more found while reviewing:
Generated by Claude Code |
…po-collision-guard # Conflicts: # CHANGELOG.md
2 participants
Summary
Fixes #123.
deriveSluguses only the trailing path segment ofsource_repo, soacme/whisperandopenai/whisperboth producewhisper, and publishing the second wrote it asv2of the first. The entry's version history then spanned two unrelated codebases, and sincebuildIndexEntryreads only the newest version's metadata,index.yamlattributed the whole entry to whichever repo published last. Nothing reported it, at publish time or after.publishEntrynow reads thesource_reporecorded on the newest version and fails before writing anything when it denotes a different repository.Comparison is normalized so re-publishing one repository spelled another way is unaffected. Scheme or none, embedded credentials (
git@,user:token@),git@host:pathSCP syntax, a default port, awww.prefix, a trailing.git, trailing slashes, backslash separators and casing all collapse to one form. A non-default port is kept, since two services on one host can differ by port alone, and only spellings that are unambiguously the same target collapse, because the caller treats "different" as fatal.Second commit: a false positive I introduced and then found
Worth reading on its own. The first pass ran the SCP branch before stripping the scheme and never stripped a userinfo prefix, so four ordinary spellings of one repository each reduced to something different:
Publishing once from an HTTPS clone and later from an SSH clone of the same repo was refused as a different project, which is the failure direction this guard exists to avoid. Reordered to strip scheme, then userinfo, then apply the SCP branch only when there was no scheme, since that is the only context where a colon means
host:pathrather than a port.The first round of tests missed it because every URL form they used had either no scheme or no credentials. That gap is closed: nine more equivalent spellings, plus distinctness assertions pinning non-default ports, Windows drive letters, and a same-host different-path pair. Two tests fail against the first commit's normalization.
A repository that genuinely moved is the one legitimate reason to change the recorded value, so
allowSourceRepoChangeopts out, exposed on MCP asallow_source_repo_change.Type of change
Checklist
npm testpasses locally).codecarto/files)feat:,fix:,docs:,refactor:,ci:,framework:)Eight new tests, 350 total, suite green from a 342 baseline. Three fail with the guard disabled: the two-project collision, the metadata-only path, and
forceNewVersion. The other five pin the normalization equivalences, keep genuinely distinct repos distinct, and hold the false-positive and malformed-metadata paths open. No.codecarto/files touched, so the invariant tests were unaffected either way, and they pass.Related issues
Fixes #123.
Found in the same audit, filed separately since each needs its own decision: #124 (the documented
confidentialityvsvisibilitycomparison does not exist), #125 (publish is documented as committing by default, butcommitPublishhas no callers), #126 (three more placeslibrary-format.mddescribes a shape the code does not produce).Additional notes
Three decisions worth your attention:
The check sits ahead of the content-hash branch. Identical spec bytes take the metadata-only path, which overwrote the other project's
source_repoandheadlinein place, so guarding only the new-version path would have left the quieter half of the bug intact.forceNewVersiondoes not bypass it. That option means "another version of this entry", not "overwrite a different project".Unreadable or malformed recorded metadata skips the check. There is nothing to compare against, and refusing on unknown would turn a corrupt
v1into an entry nobody can publish to.On the doc change:
library-format.mdpromised auto-suffixing (-2,-3) for this case. It was never implemented, and silent suffixing is its own kind of surprise for an agent-driven tool, so the doc now describes the refusal and the override rather than the behaviour we would have had to build to match it.Deliberately not in this PR
The escape hatch is MCP only, which is the opposite of the surface priority in CLAUDE.md. Closing it on Pi is UX work on the primary surface and wanted its own discussion rather than a ride-along in a bug fix. Two pre-existing Pi gaps, neither caused by this change:
/codecarto-publishtakes no arguments (handler: async (_args, ctx)), so a Pi user who trips the guard after genuinely moving a directory gets a clear error with no way to act on it. Pi already usesctx.ui.confirmfor the publish preview, which looks like the right place to surface the conflict and offer the override.Pi passes
ctx.cwdassource_repo, a local absolute path. That is also why the collision is easy to hit there:slugandsource_repoboth derive fromctx.cwd, so two same-named directories under different parents collide. The format doc calls local paths "permitted but discouraged for shared libraries", and using the git remote when there is one looks like the better default.