Repository navigation
feat(sync): compile and publish repository sources - #994
Conversation
eaf8d23 to
9c1ef3d
Compare
124b85b to
5b0eb9e
Compare
5b0eb9e to
a260389
Compare
a260389 to
4952ca9
Compare
e893968 to
6509da2
Compare
6509da2 to
619db60
Compare
50306f7 to
ee3a323
Compare
ee3a323 to
a1acb87
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
a1acb87 to
ec06ea8
Compare
nkester-ld
left a comment
nkester-ld
left a comment
There was a problem hiding this comment.
This seems like an actually big and important bug to cover. There was one other bugs I found with failing tests (AI wrote this? No I wrote this) , but it seems like an edge edge case.
When a file under .launchdarkly is a broken symlink, such as a dangling link after a checkout, workspaceFS.Open resolves the link before checking for symlinks and gets "not found". That makes a broken search.json link report "no local definition, run ldcli sync tools add", and a broken search.ref.yaml beside a valid search.json is silently ignored, so the local file gets compiled.
| }) | ||
| require.ErrorContains(t, err, "8 MiB") | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd love the test coverage case where an authored Python or TypeScript file has an unrelated relative import of a generated module covered. I created this test that seems to fail one is present:
.launchdarkly/project/configs/support/default.ref.yaml: source is generated, vendored, or minified. Link the authored source file.
| func TestCompileWorkspaceIgnoresAnUnrelatedGeneratedImport(t *testing.T) { | |
| root := t.TempDir() | |
| writeTestFile(t, root, "app/agent.py", "from agents import Agent\nfrom .schema_pb2 import SCHEMA\n"+ | |
| "support = Agent(instructions=\"Help with orders.\")\n") | |
| writeTestFile(t, root, "app/schema_pb2.py", "# Code generated by protoc. DO NOT EDIT.\nSCHEMA = {}\n") | |
| writeTestFile(t, root, ".launchdarkly/project/configs/support/default.ref.yaml", | |
| "formatVersion: 1\nfile: app/agent.py\nformat: python\nselect: 'agents[id=\"support\"]'\n"+ | |
| "map:\n prompt: instructions\n") | |
| resources, err := CompileWorkspace(root) | |
| require.NoError(t, err) | |
| require.Len(t, resources, 1) | |
| assert.Equal(t, "Help with orders.", resources[0].Variation.Instructions) | |
| } |
There was a problem hiding this comment.
Fixed, and thank you for the test. A generated, vendored, or minified file now reads as a read-only dependency instead of failing the resource. readSourceDocument and the publication snapshot mark such a file with Document.ReadOnly, and code.LocateConstants makes every value that an import resolves in it read-only, with the reason <file> is generated, vendored, or minified, so sync does not write it, so a mapped value in it blocks the resource at plan time. The primary file of a descriptor still blocks the resource, and staging still rejects a write to such a file. Your test is in as TestCompileWorkspaceIgnoresAnUnrelatedGeneratedImport. TestCompileSourceMarksAValueInAGeneratedDependencyReadOnly covers a mapped value in a generated module, a link into a vendored tree, and an authored dependency that stays direct, and TestLocateConstantsMakesTheValuesOfAReadOnlyDependencyReadOnly covers the helper.
Compile descriptors into projections with their repository dependencies. Publish pulls as one unit: one byte snapshot per file, one edit pass per file, a reparse of every target, a check of every other binding that reads an edited file, and one atomic replacement. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit b262b74. Configure here.
| return stagedFile{ | ||
| displayPath: file, path: path, original: original, originalExists: true, | ||
| replacement: updated, mode: info.Mode().Perm(), sources: names, root: store.repositoryRoot, | ||
| }, nil |
There was a problem hiding this comment.
Staging re-resolves snapshot source paths
High Severity
stageSourceDocument calls resolveSourcePath again instead of writing to the path the snapshot already resolved. A link retargeted after that read can make the commit's name check compare the new target to itself. When the new file still has the snapshot bytes, the publish writes the old file's edits onto the new target.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b262b74. Configure here.
| if !slices.Contains(snapshot.names[key], file) { | ||
| snapshot.names[key] = append(snapshot.names[key], file) | ||
| } | ||
| return syncadapter.Document{Path: file, Data: document.Data}, nil |
There was a problem hiding this comment.
Snapshot cache drops generated ReadOnly
Medium Severity
A cached sourceSnapshot.document returns only Path and Data, so ReadOnly from the first read is dropped. A later bundle of the same generated file (another descriptor, another name, or a primary after a dependency) treats it as writable, and read() will not reject it as a generated primary.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b262b74. Configure here.
Context
Descriptor-backed sources need compilation into projections and a publication path that cannot lose or corrupt an edit.
What changes
Design notes
Review focus
Verification
make buildgo test ./...golangci-lint run ./...Related changes
🤖 Generated with Claude Code
Note
Overview
Adds descriptor-backed source sync:
CompileWorkspace/CompileAttachmentsnow compile.ref.yamldefinitions (variations, tools, skills) through registered format adapters, resolve static dependencies, and reject conflicting managed-file vs descriptor definitions, unsafe paths, and symlink-only managed entries.Introduces
ReplaceSources/CheckSources— a single publication unit with per-file snapshots, grouped text edits, post-edit verification, descriptor mapping guards, and atomicreplaceFiles; edits refuse stale fingerprints, overlapping changes, read-only/generated targets, and changes that would alter non-target descriptors sharing a file.Descriptor relationship pulls update top-level
tools/skillsin place (preserving comments when possible) or re-render when YAML layout blocks in-place edits; missing local attachment defs can be materialized during pull when content is supplied.SourceFilesnow tracks referenced sources and resolved symlink targets for watch/planning;ProjectionIncludescompares projections that omit field states.Reviewed by Cursor Bugbot for commit b262b74. Bugbot is set up for automated code reviews on this repo. Configure here.