Repository navigation
fix(cloud): preserve literal project identities (#1583) - #1590
Conversation
…tleman-Programming#1583) Azure DevOps renders spaces in repo names as %20, so git_remote detection stored observations under the encoded spelling (my%20project) while `engram cloud enroll` applies a single url.PathUnescape to its argument and enrolled the decoded spelling (my project). The two identities never met and sync stayed blocked. extractRepoName now decodes the repo-name segment exactly once, mirroring the enroll CLI's single-unescape policy, and keeps names with invalid escapes raw. Existing repository bindings are reused unchanged; the explicit convergence paths for stores that already hold the encoded spelling are an explicit project_name override or enrolling the double-encoded form.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughProject detection now decodes valid percent escapes in the final Git remote URL component once. Tests cover encoded Azure DevOps names, double-encoded names, malformed escapes, and decoded project identity. ChangesProject Name Detection
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Cloud users can explicitly select percent-encoded project identities while default decoding remains available. The supplied coverage checks that literal and decoded targets stay separate, and no actionable merge-blocking risk is identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Literal project selection retains normalization, enrollment checks, and project-specific synchronization. The inspected behavior keeps encoded and decoded identities distinct, with no demonstrated new security boundary bypass. Production authorization and interrupted end-to-end synchronization were not verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Resolution Make detection and the default cloud project flow produce the same identity, or change the linked issue's requirement to accept explicit literal selection. Add regression tests that exercise the reported Azure DevOps
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dnlrsls
left a comment
dnlrsls
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. I tested commit 60c7d843 in an isolated checkout: the new Azure regression passes, and existing encoded bindings remain stable. Before merging, please address these identity edge cases:
- Reject decoded separators before fallback normalization. In
internal/project/detect.go,PathUnescapefeedsnormalize(name), which converts names containing/or\tounknownbefore binding validation. Bothhttps://example.test/team/alpha%2Fbeta.gitandhttps://example.test/team/gamma%5Cdelta.gitcreated a persistentunknownbinding with no error in my probes. This contradicts the PR's fail-closed claim and can make unrelated repos select the same project bucket. Add full-detection tests that reject these inputs without creating a binding, while preserving valid existing bindings. This is an identity-collision concern, not a demonstrated cloud authorization bypass. - Cover history that predates the private binding. Binding absence does not establish that the project is new. With an existing observation under
my%20projectand no Git binding, first detection now selectsmy project; a subsequent write leaves one observation in each separate bucket. Please add this upgrade regression and define a safe, explicit policy for this case. Preserving existing bindings and advising configuration changes does not by itself prevent this silent split. - Preserve literal local-remote paths.
/local/repos/my%20project.gitnow extracts asmy project, although%20can be part of the actual filesystem name. Distinguish local-path syntax from encoded URI input and add a regression for literal percent sequences. - Test the complete detection-to-enrollment contract for double encoding.
my%2520project.gitdetects asmy%20project, but enrolling that exact displayed name selectsmy project. The new extraction test proves a single decode, not end-to-end identity consistency. Please test this boundary and document or explicitly support literal project selection rather than claiming all names converge.
The PR tests and these additional probes were run with go test ./internal/project ./cmd/engram -run '^(TestExtractRepoName|TestDetectProject_GitRemote_PercentEncoded|TestReview1590.*)$' -count=1 -v -timeout=60s. The added probe tests were temporary and are not part of this PR. No user database or external cloud service was modified.
dnlrsls
added
type:bug
dnlrsls
changed the title
Follow-up to dd7fd7d: the rewritten enroll/sync literal-project tests left four unchecked defer s.Close calls and one unchecked fmt.Fprint, which the Lint job flags. Check them explicitly.
|
Thanks for the thorough review and for dd7fd7d, which resolves all four edge cases by the stronger route: detection stays literal, existing detected buckets are never migrated, and identity selection moves to the explicit Under that design the cases I had started fixing dissolve rather than need patching: decoded separators cannot reach normalization (no decoding), pre-binding history cannot split (detection is unchanged), local filesystem remotes are literal by construction, and the double-encoding boundary is now a documented selection concern in DOCS.md instead of a convergence claim. I dropped my in-progress decoding follow-up accordingly. Verified on dd7fd7d locally: build, One thing I could add: the Lint job on dd7fd7d flags four unchecked |
Original findings are addressed in a6af4ea: detected identities remain unchanged and --literal-project provides explicit selection without migrations. Exact-head independent validation and four-lens native review passed; CI is green. Withdrawing my original change request, not approving: I contributed dd7fd7d, so another maintainer must review. No merge performed.
a43183c
Maintainer correction
Closes #1583
The correction in
dd7fd7d1supersedes the original remote-decoding approach below. The original author commit remains in history; this was a normal fast-forward update, not a rewrite.--literal-projectfor cloud enrollment, unenrollment, and explicit cloud sync. Default single URL decoding remains unchanged.Verification for the corrected candidate
go test ./internal/project -count=1 -timeout=60s: passed (14.267s).cmd/engramliteral, default-decoding, parser, help, and cloud-status regressions: passed (3.995s).gofmtandgit diff --check: clean.Overall PR diff at a6af4ea: 421 authored changed lines. Maintainer explicitly authorized an exception up to 440;
size:exceptionandtype:bugare applied.Correction AI assistance: Pi coding agent authored the correction and tests under human-selected conservative design, size-exception, and push authorization. An independent verifier inspected the candidate and provider-owned native review reviewed immutable trees.
Original submission, retained for provenance (applies to initial commit 60c7d84 only)
🔗 Linked Issue
Closes #1583
🏷️ PR Type
type:bug— Bug fixLabel request: contributor is pull-only and cannot apply
type:bug; requesting maintainer application.📝 Summary
extractRepoNamenow decodes the repo-name segment of a git remote URL exactly once (url.PathUnescape), mirroring the single-unescape policyengram cloud enrollalready applies to its argument (cmd/engram/cloud.go). Azure DevOps remotes likehttps://org.visualstudio.com/My%20Project/_git/My%20Projectnow detect asmy project, so observations and sync enrollment converge on one project identity instead of forkingmy%20projectvsmy projectand blocking sync.normalizeProjectName).project_nameexplicitly in.engram/config.json, or enroll the double-encoded form (engram cloud enroll "my%2520project"), which single-unescapes to the encoded identity.📂 Changes
internal/project/detect.goextractRepoName, with raw fallback on invalid escapesinternal/project/detect_test.goextractRepoNametable cases (Azure HTTPS/SSH v3 encoded space, double-encoded decodes once, invalid escape raw) +TestDetectProject_GitRemote_PercentEncodedregression proving detection yields the decoded canonical identity🤖 AI Assistance
Select exactly one option. Do not check both options.
Tool/model (if known): Pi coding agent (Claude Sonnet 4.5).
Material scope: Fix and tests authored by the AI agent under direct human instruction (claim decision, plan approval, and push/PR authorization were human).
Verification performed: Test-first (RED on all 4 new failing assertions reproducing the issue split, then GREEN); full
go test ./...exit=0;go vet ./internal/project/clean;gofmtclean;go build ./...OK.🧪 Test Plan
Unit Tests
Go Format
go test ./...)my%20projectvsmy project) before the fix✅ Contributor Checklist
status:approved(Project name from git remote keeps URL-encoding (%20), but cloud enroll decodes it, so sync is blocked #1583)type:*label to the PR (cannot: pull-only; requested above)go test ./...)Co-Authored-BytrailersSummary by CodeRabbit
--literal-projectto preserve percent-encoded characters. For sync, it requires cloud sync and one explicit, non-empty--project; it cannot be combined with--all.