feat(sync): Slice 6 — sync adapters, tool injection, and CLI#30
Open
feat(sync): Slice 6 — sync adapters, tool injection, and CLI#30
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Sync & Inject subsystem: protobuf API, storage types and memgraph backend, adapter framework (Beads, GitHub) with command runners, server ConnectRPC handlers, CLI commands, an ExecRunner, and an injection engine that renders and writes tool-specific spec artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/User
participant Client as SyncService Client
participant Handler as SyncHandler
participant SpecStore as Spec Store
participant SyncStore as Sync Store
participant Adapter as Adapter (Beads/GitHub)
participant Runner as ExecRunner/CLI
CLI->>Client: SyncBeads/SyncGitHub request
Client->>Handler: RPC
Handler->>SpecStore: ListSpecs(stage, priority)
SpecStore-->>Handler: [specs...]
loop per spec
Handler->>SyncStore: GetSyncMapping(spec, adapter)
alt mapping exists
Handler-->>Handler: skip
else
Handler->>Adapter: Available()
Adapter->>Runner: run CLI (bd/gh)
Runner-->>Adapter: output (JSON)
Adapter-->>Handler: externalID
Handler->>SyncStore: CreateSyncMapping(spec, externalID)
SyncStore-->>Handler: SyncMapping
end
end
Handler-->>Client: SyncResponse
Client-->>CLI: print results
sequenceDiagram
participant CLI as CLI/User
participant Client as SyncService Client
participant Handler as SyncHandler
participant SpecStore as Spec Store
participant ConstStore as Constitution Store
participant Injector as Inject package
participant FS as File System
CLI->>Client: InjectRequest(specSlug, tool, outputDir)
Client->>Handler: RPC
Handler->>SpecStore: GetSpec(slug)
SpecStore-->>Handler: Spec
Handler->>ConstStore: GetConstitution() (optional)
ConstStore-->>Handler: Constitution
Handler->>Injector: Inject(spec, constitution, tool, outputDir)
Injector->>FS: create dirs & write files
FS-->>Injector: written file paths
Injector-->>Handler: files list & summary
Handler-->>Client: InjectResponse
Client-->>CLI: print summary & files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Owner
Author
PR Review — Slice 6: sync adapters, tool injection, and CLI4 findings: 2 critical · 2 important Critical
The function creates a
Important
If
The Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
internal/sync/beads_test.go (1)
15-22: Make the mock verify the CLI invocation, not just the output.Right now
mockRunnerignoresnameandargs, so these tests still pass if the adapter starts calling the wrong binary or flag set. Since this adapter is mostly command construction plus JSON parsing, capturing the invocation here would make the suite much harder to fool.Suggested direction
type mockRunner struct { output []byte err error + name string + args []string } -func (m *mockRunner) Run(_ context.Context, _ string, _ ...string) ([]byte, error) { +func (m *mockRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { + m.name = name + m.args = append([]string(nil), args...) return m.output, m.err }Then assert
name/argsin the happy-path tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sync/beads_test.go` around lines 15 - 22, Update mockRunner so it records and/or validates the CLI invocation instead of ignoring name and args: modify the struct mockRunner to include fields for receivedName and receivedArgs (and optionally expectedName/expectedArgs), update the Run method to capture the incoming name and args (or assert they match expected values) before returning output/err, and then update the happy-path tests to assert the adapter invoked the correct binary and flags by checking mockRunner.receivedName and mockRunner.receivedArgs (or relying on the Run assertions). Ensure references to mockRunner and its Run(ctx, name, args...) method are used to locate and change the code.internal/inject/inject.go (1)
60-66: Non-deterministic output due to map iteration order.Iterating over
con.Tech.Frameworks(a map) produces output in random order, which can cause test flakiness and confusing diffs in generated files. Consider sorting the keys for consistent output.♻️ Proposed fix using sorted keys
+ "sort"if con.Tech != nil && len(con.Tech.Frameworks) > 0 { b.WriteString("### Frameworks\n\n") + keys := make([]string, 0, len(con.Tech.Frameworks)) for k, v := range con.Tech.Frameworks { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + v := con.Tech.Frameworks[k] fmt.Fprintf(&b, "- **%s:** %s\n", k, v) } b.WriteString("\n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/inject/inject.go` around lines 60 - 66, The code iterates over the map con.Tech.Frameworks causing non-deterministic order; to fix, collect the map keys into a slice, sort them (e.g., using sort.Strings), and then iterate over the sorted keys to write entries into b so the output from the block that writes "### Frameworks" is stable and deterministic.proto/specgraph/v1/sync.proto (1)
55-60: Consider using enum types for filter fields inSyncConfig.
filter_stageandfilter_priorityare strings, but the codebase likely has predefined stage and priority values. Using enums would provide compile-time type safety and better documentation. However, strings offer flexibility if these values are user-defined or extensible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/specgraph/v1/sync.proto` around lines 55 - 60, The SyncConfig message currently uses string fields filter_stage and filter_priority which should be strong-typed enums for known values; define proto enums (e.g., FilterStage and FilterPriority) in the same proto file with the allowed values, change SyncConfig's filter_stage and filter_priority field types to those enum types, and update any call sites and generated code references that construct or read SyncConfig (e.g., places creating SyncConfig or checking .filter_stage/.filter_priority) to use the enum symbols instead of raw strings so compilation and serialization reflect the new types.internal/server/sync_handler.go (2)
113-129: Consider sanitizing error messages in sync results.Raw error messages from
adapter.Push(line 116) andCreateSyncMapping(line 125) are included directly inresult.Message. While these help with debugging, they may expose internal details to clients. Consider whether these messages need sanitization based on your security posture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/sync_handler.go` around lines 113 - 129, The code currently assigns raw error strings from adapter.Push and h.syncStore.CreateSyncMapping into result.Message (see variables pushErr and createErr and the field result.Message); change this to store a sanitized/generic message for clients (e.g., "sync failed" or "internal error during push/create mapping") and increment resp.Errors as before, while logging the original pushErr/createErr details to your server logs or monitoring (use the existing logger/contextual logger) for diagnostics; ensure result.Message does not expose internal stack traces or sensitive data and preserve externalID/adapter.Name() behavior for successful flows.
186-189: Silently ignoring constitution errors could mask real issues.While the comment explains constitution is optional, silently ignoring all errors (including connectivity issues, timeouts, etc.) could make debugging difficult. Consider logging the error at debug level for observability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/sync_handler.go` around lines 186 - 189, The code silently discards errors from h.constitutionStore.GetConstitution which can hide connectivity/timeout issues; change the call to capture the error (e.g., constitution, err := h.constitutionStore.GetConstitution(ctx)) and if err != nil log it at debug/trace level using the handler's logger (e.g., h.logger.Debugf or h.log.Debug) while preserving the current behavior of treating the constitution as optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/specgraph/serve.go`:
- Line 97: The call to server.RegisterSyncService currently passes only (mux,
store, store, store) and thus constructs a SyncHandler with an empty adapters
map; wire actual adapter implementations for SyncBeads and SyncGitHub into that
registration so the handler has real backends. Locate server.RegisterSyncService
and change the fourth/fifth argument usage to pass a populated adapters map (or
construct one before calling) that includes concrete implementations (e.g.,
BeadsAdapter, GitHubAdapter or whatever types are provided by your codebase)
keyed by the names used by SyncHandler; ensure the adapters map passed to
SyncHandler in internal/server/sync_handler.go is not empty so SyncBeads and
SyncGitHub routes resolve to real adapters.
In `@cmd/specgraph/sync.go`:
- Around line 111-117: The adapter handling silently leaves adapter set to
specv1.SyncAdapter_SYNC_ADAPTER_UNSPECIFIED for unknown statusAdapter values;
change the switch to validate input and return an error for any non-recognized
adapter (e.g., after normalizing case with strings.ToLower) instead of
defaulting to UNSPECIFIED, so a typo like "gihub" fails fast; update the code
referencing statusAdapter and adapter (the switch block creating adapter and
using specv1.SyncAdapter_SYNC_ADAPTER_BEADS / _GITHUB) to return a descriptive
error when the value is not "beads" or "github" and add the strings import if
you use strings.ToLower.
In `@internal/inject/inject.go`:
- Around line 105-109: The YAML front matter is built with strings.Builder and
fmt.Fprintf using spec.Intent directly (see variable b and the fmt.Fprintf
line), which can break YAML if spec.Intent contains quotes, newlines, or colons;
change this to produce a safe YAML string by escaping or serializing the
description value instead of interpolating raw text — e.g., serialize the
description map with a YAML library or call a string-quoting/escaping utility
(or strconv.Quote) on spec.Intent before writing it into the builder so the
description field always produces valid YAML.
- Around line 18-31: Inject currently calls renderMarkdown(spec, constitution)
without validating spec; add a nil guard at the start of Inject to check if spec
== nil and return a clear error (e.g., fmt.Errorf("nil spec") or similar) before
calling renderMarkdown, so functions like renderMarkdown and downstream code
that access spec.Slug (and writeCursor/writeClaudeCode) cannot panic when spec
is nil.
In `@internal/server/convert.go`:
- Around line 664-699: Change the enum converter functions to return explicit
errors for unknown/unsupported values rather than silently falling back: update
syncAdapterFromProto to return (storage.SyncAdapterType, error) and return an
error when the incoming specv1.SyncAdapter is not a known case (instead of ""),
and update syncStateToProto to return (specv1.SyncState, error) and return an
error for unknown storage.SyncStateType (instead of SYNC_STATE_UNSPECIFIED);
follow the pattern used in decisionStatusFromProto and edgeTypeFromProto for
error messages and types, and then update callers (e.g., GetSyncStatus and any
other usages) to handle/propagate these errors and validate enum conversion
before passing values into storage or downstream code.
In `@internal/server/sync_handler.go`:
- Around line 86-89: The handler currently returns the raw error from
h.specStore.ListSpecs (connect.NewError(connect.CodeInternal, err)), which may
leak internal details; replace this by wrapping/sanitizing the error using the
project's error helper (e.g., call stageError or another sanitizer) to return a
generic internal error to clients while logging the original internally; locate
the call to h.specStore.ListSpecs in sync_handler.go and change the error path
to return stageError(ctx, "failed to list specs", err) or the equivalent
sanitizing wrapper instead of directly returning err.
In `@internal/storage/memgraph/sync.go`:
- Around line 191-204: DeleteSyncMapping currently silently succeeds when no
mapping exists; modify it to detect and return ErrSyncMappingNotFound by
changing the Cypher to return the deleted relationship count (e.g., `MATCH
(s:Spec {slug: $slug})-[r:SYNCED_TO {adapter: $adapter}]->(e:ExternalRef) DELETE
r, e RETURN count(r) AS deleted`) and inspect the query result from
s.executeQuery in DeleteSyncMapping; if deleted == 0 return
storage.ErrSyncMappingNotFound, otherwise proceed as normal, preserving the
existing error wrapping for query execution.
In `@internal/sync/beads.go`:
- Around line 62-67: After json.Unmarshal into beadsCreateResponse / the pull
response struct, add explicit guards that treat empty required fields as
failures: verify resp.ID != "" in the create path and resp.Status != "" (or
other required fields) in the pull path and return ErrPushFailed / ErrPullFailed
with a wrapped error if they're empty; update the code around
beadsCreateResponse, resp.ID, ErrPushFailed and the corresponding pull response
check (resp.Status, ErrPullFailed) to return a descriptive fmt.Errorf("%w: empty
<field> in response", ErrPushFailed/ErrPullFailed) instead of succeeding on an
empty value.
In `@internal/sync/github.go`:
- Around line 56-72: The code calls g.runner.Run with "gh issue create"
including a non-existent "--json" flag and then attempts to json.Unmarshal the
plain-text URL in out into ghCreateResponse (resp), which will always fail; fix
by either (A) keeping "gh issue create" and parsing the returned URL string in
out to extract the issue number (replace the json.Unmarshal and
strconv.Itoa(resp.Number) usage with URL parsing logic and ensure errors are
handled), or (B) switch to calling the GitHub REST endpoint via "gh api
repos/OWNER/REPO/issues" with the same title/body/labels payload so the command
returns JSON you can unmarshal into ghCreateResponse and keep the existing
strconv usage; update the error messages around g.runner.Run and the Unmarshal
branch accordingly to reference out and the chosen approach.
---
Nitpick comments:
In `@internal/inject/inject.go`:
- Around line 60-66: The code iterates over the map con.Tech.Frameworks causing
non-deterministic order; to fix, collect the map keys into a slice, sort them
(e.g., using sort.Strings), and then iterate over the sorted keys to write
entries into b so the output from the block that writes "### Frameworks" is
stable and deterministic.
In `@internal/server/sync_handler.go`:
- Around line 113-129: The code currently assigns raw error strings from
adapter.Push and h.syncStore.CreateSyncMapping into result.Message (see
variables pushErr and createErr and the field result.Message); change this to
store a sanitized/generic message for clients (e.g., "sync failed" or "internal
error during push/create mapping") and increment resp.Errors as before, while
logging the original pushErr/createErr details to your server logs or monitoring
(use the existing logger/contextual logger) for diagnostics; ensure
result.Message does not expose internal stack traces or sensitive data and
preserve externalID/adapter.Name() behavior for successful flows.
- Around line 186-189: The code silently discards errors from
h.constitutionStore.GetConstitution which can hide connectivity/timeout issues;
change the call to capture the error (e.g., constitution, err :=
h.constitutionStore.GetConstitution(ctx)) and if err != nil log it at
debug/trace level using the handler's logger (e.g., h.logger.Debugf or
h.log.Debug) while preserving the current behavior of treating the constitution
as optional.
In `@internal/sync/beads_test.go`:
- Around line 15-22: Update mockRunner so it records and/or validates the CLI
invocation instead of ignoring name and args: modify the struct mockRunner to
include fields for receivedName and receivedArgs (and optionally
expectedName/expectedArgs), update the Run method to capture the incoming name
and args (or assert they match expected values) before returning output/err, and
then update the happy-path tests to assert the adapter invoked the correct
binary and flags by checking mockRunner.receivedName and mockRunner.receivedArgs
(or relying on the Run assertions). Ensure references to mockRunner and its
Run(ctx, name, args...) method are used to locate and change the code.
In `@proto/specgraph/v1/sync.proto`:
- Around line 55-60: The SyncConfig message currently uses string fields
filter_stage and filter_priority which should be strong-typed enums for known
values; define proto enums (e.g., FilterStage and FilterPriority) in the same
proto file with the allowed values, change SyncConfig's filter_stage and
filter_priority field types to those enum types, and update any call sites and
generated code references that construct or read SyncConfig (e.g., places
creating SyncConfig or checking .filter_stage/.filter_priority) to use the enum
symbols instead of raw strings so compilation and serialization reflect the new
types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 134e7165-4fa1-47d6-b5f8-acceac8d1f3a
⛔ Files ignored due to path filters (2)
gen/specgraph/v1/specgraphv1connect/sync.connect.gois excluded by!**/gen/**gen/specgraph/v1/sync.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (20)
cmd/specgraph/inject.gocmd/specgraph/serve.gocmd/specgraph/sync.gointernal/inject/inject.gointernal/inject/inject_test.gointernal/server/convert.gointernal/server/sync_handler.gointernal/server/sync_handler_test.gointernal/storage/memgraph/memgraph.gointernal/storage/memgraph/sync.gointernal/storage/memgraph/sync_test.gointernal/storage/sync.gointernal/sync/adapter.gointernal/sync/beads.gointernal/sync/beads_test.gointernal/sync/exec.gointernal/sync/exec_test.gointernal/sync/github.gointernal/sync/github_test.goproto/specgraph/v1/sync.proto
…d ConnectRPC handler Includes CodeRabbit autofix: - Return *SyncHandler from RegisterSyncService; wire BeadsAdapter in serve.go - Fix gh issue create: parse URL output instead of unsupported --json flag - Enum converters return errors for unknown values - Sanitize internal errors before returning to clients - Handle GetSyncMapping errors explicitly - DeleteSyncMapping returns ErrSyncMappingNotFound when no mapping exists - Reject empty fields after JSON decode in beads adapter - Reject unknown --adapter values in sync status CLI - Add nil guard for spec parameter in Inject - Escape quotes in spec.Intent for YAML safety
seanb4t
force-pushed
the
feat/slice-6-sync-integration
branch
from
2ce66e7 to
f318831
Compare
Owner
Author
Fixes Applied SuccessfullyFixed 9 files based on 9 unresolved CodeRabbit review comments. Files modified:
Commit: The latest autofix changes are on the |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
internal/inject/inject.go (1)
108-111:⚠️ Potential issue | 🟡 MinorCursor front matter escaping is still incomplete.
Escaping
"alone is not enough here. Intents with backslashes or embedded newlines can still produce invalid or mangled YAML front matter. Serialize the full description string instead of hand-escaping it.🧾 Safer quoting
import ( "fmt" "os" "path/filepath" + "strconv" "strings" @@ - safeIntent := strings.ReplaceAll(spec.Intent, `"`, `\"`) + description := strconv.Quote(fmt.Sprintf("SpecGraph spec %s: %s", spec.Slug, spec.Intent)) var b strings.Builder b.WriteString("---\n") - fmt.Fprintf(&b, "description: \"SpecGraph spec %s: %s\"\n", spec.Slug, safeIntent) + fmt.Fprintf(&b, "description: %s\n", description) b.WriteString("alwaysApply: false\n") b.WriteString("---\n\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/inject/inject.go` around lines 108 - 111, The current code manually escapes quotes for spec.Intent and writes YAML front matter with fmt.Fprintf (safeIntent, strings.Builder b), which fails for backslashes/newlines; instead build the full description string (e.g., desc := fmt.Sprintf("SpecGraph spec %s: %s", spec.Slug, spec.Intent)) and serialize it using a robust serializer (for example strconv.Quote(desc) or yaml.Marshal(desc) and strip surrounding document markers) before writing into the builder so all quotes, backslashes and newlines are correctly escaped; replace the safeIntent variable and the fmt.Fprintf call accordingly.cmd/specgraph/serve.go (1)
98-99:⚠️ Potential issue | 🔴 Critical
SyncGitHubis still unconfigured in the server.This only registers the Beads adapter. The handler returned by
RegisterSyncServicestill has no GitHub backend, so the newsync githubflow will continue returning “adapter not configured” at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/specgraph/serve.go` around lines 98 - 99, The sync handler returned by RegisterSyncService is only having the Beads adapter registered (via syncHandler.RegisterAdapter) but the GitHub backend (SyncGitHub) is not configured, causing “adapter not configured” at runtime; update the initialization to also configure the GitHub backend on syncHandler—e.g. after syncHandler.RegisterAdapter(...) call the appropriate method to attach the GitHub implementation (such as syncHandler.RegisterGitHub or syncHandler.SetGitHubBackend) and pass a properly constructed GitHub adapter/client (using the existing syncpkg GitHub adapter/factory) so the `sync github` flow is fully wired.
🧹 Nitpick comments (1)
internal/server/convert.go (1)
707-718:injectToolFromProtoshould return an error for unknown values, consistent with other converters.The other
*FromProtoconverters (syncAdapterFromProto,syncStateFromProto,decisionStatusFromProto,edgeTypeFromProto) return errors for unrecognized values. This function silently converts unknown proto enum values to"", which is inconsistent. Since proto3 enums are open, a client could send an unknown numeric value that bypasses theINJECT_TOOL_UNSPECIFIEDvalidation in the handler.♻️ Proposed fix for consistency
-func injectToolFromProto(t specv1.InjectTool) storage.InjectToolType { +func injectToolFromProto(t specv1.InjectTool) (storage.InjectToolType, error) { switch t { case specv1.InjectTool_INJECT_TOOL_CLAUDE_CODE: - return storage.InjectToolClaudeCode + return storage.InjectToolClaudeCode, nil case specv1.InjectTool_INJECT_TOOL_CURSOR: - return storage.InjectToolCursor + return storage.InjectToolCursor, nil case specv1.InjectTool_INJECT_TOOL_AGENTS_MD: - return storage.InjectToolAgentsMD + return storage.InjectToolAgentsMD, nil default: - return "" + return "", fmt.Errorf("unknown inject tool: %v", t) } }Update the caller in
sync_handler.goto handle the error:tool, err := injectToolFromProto(req.Msg.Tool) if err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/convert.go` around lines 707 - 718, Change injectToolFromProto to return (storage.InjectToolType, error) instead of silently returning "" for unknown enum values: keep the existing case mappings (InjectTool_INJECT_TOOL_CLAUDE_CODE -> storage.InjectToolClaudeCode, INJECT_TOOL_CURSOR -> storage.InjectToolCursor, INJECT_TOOL_AGENTS_MD -> storage.InjectToolAgentsMD) and in the default branch return a non-nil error describing the unknown value; update the caller in sync_handler.go to use the new signature (e.g., tool, err := injectToolFromProto(req.Msg.Tool)) and return connect.NewError(connect.CodeInvalidArgument, err) when err != nil so unknown proto numeric values are rejected consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/inject/inject.go`:
- Around line 90-99: The writeClaudeCode function (and the sibling slug-based
writers referenced around lines 102-120) uses spec.Slug directly to build
filenames; validate and sanitize the slug before creating paths: reject or
normalize any slug that contains path separators or traversal segments (e.g.,
".." or "/" / "\"), ensure the slug is non-empty and consists only of allowed
characters (or compare filepath.Base(slug) == slug), and return an error if
validation fails so you never join an untrusted slug into outputDir; update
writeClaudeCode (and the other slug writers) to perform this check before
calling filepath.Join or writing files.
In `@internal/server/sync_handler_test.go`:
- Around line 97-103: The mock DeleteSyncMapping currently always returns nil;
update mockSyncBackend.DeleteSyncMapping to check if the computed key (via
m.key(specSlug, adapter)) exists in m.mappings under the mutex and, if not
found, return the sentinel error storage.ErrSyncMappingNotFound (instead of nil
or fmt.Errorf), otherwise delete and return nil so tests can exercise the
handler's not-found path; reference mockSyncBackend.DeleteSyncMapping, m.key,
m.mappings, and storage.ErrSyncMappingNotFound.
In `@internal/server/sync_handler.go`:
- Around line 181-210: The handler returns raw internal errors from
h.specStore.GetSpec, os.Getwd and inject.Inject directly to clients; replace
those raw errors with sanitized client-facing messages while logging the
original error for debugging: for h.specStore.GetSpec keep the not-found branch
as-is but when err != nil and not ErrSpecNotFound, log the err (including
req.Msg.SpecSlug) and return connect.NewError(connect.CodeInternal,
errors.New("failed to load spec")); for the os.Getwd failure, log wdErr and
return connect.NewError(connect.CodeInternal, errors.New("failed to determine
working directory")); and for inject.Inject failures, log the inject error
(include spec slug and tool via injectToolFromProto) and return
connect.NewError(connect.CodeInternal, errors.New("injection failed")); ensure
logs use the existing logger and do not change the external error codes.
- Around line 122-138: The code is leaking internal error details by assigning
pushErr.Error() and createErr.Error() directly to result.Message; update the
handling in the adapter.Push and h.syncStore.CreateSyncMapping failure blocks to
log the original errors (using the existing logger) and set result.Message to a
sanitized, generic message (e.g., "internal error while syncing with adapter" or
"internal error creating sync mapping") while preserving contextual info such as
adapter.Name() and spec.Slug in the log; keep the result.State =
specv1.SyncState_SYNC_STATE_ERROR and increment resp.Errors/append resp.Results
as before.
In `@internal/storage/memgraph/sync.go`:
- Around line 37-48: The duplicate-check-then-create must be made atomic:
replace the separate SELECT-then-CREATE flow (the call to s.executeQuery that
reads existingRecords and the subsequent CREATE logic around the sync mapping in
sync.go) with a single atomic Cypher statement that either (a) uses MERGE on the
(Spec)-[r:SYNCED_TO {adapter: $adapter}]->(ExternalRef) relationship and detects
whether it was newly created vs already existing, or (b) performs a CREATE ...
WHERE NOT EXISTS(...) pattern and returns a conflict indicator; then inspect
that single-statement result and return storage.ErrSyncMappingExists when the
relationship already exists. Apply the same change to the other block mentioned
(the code handling creation between lines ~50-78) so both creation paths are
atomic and rely on one s.executeQuery call to decide success vs
ErrSyncMappingExists.
---
Duplicate comments:
In `@cmd/specgraph/serve.go`:
- Around line 98-99: The sync handler returned by RegisterSyncService is only
having the Beads adapter registered (via syncHandler.RegisterAdapter) but the
GitHub backend (SyncGitHub) is not configured, causing “adapter not configured”
at runtime; update the initialization to also configure the GitHub backend on
syncHandler—e.g. after syncHandler.RegisterAdapter(...) call the appropriate
method to attach the GitHub implementation (such as syncHandler.RegisterGitHub
or syncHandler.SetGitHubBackend) and pass a properly constructed GitHub
adapter/client (using the existing syncpkg GitHub adapter/factory) so the `sync
github` flow is fully wired.
In `@internal/inject/inject.go`:
- Around line 108-111: The current code manually escapes quotes for spec.Intent
and writes YAML front matter with fmt.Fprintf (safeIntent, strings.Builder b),
which fails for backslashes/newlines; instead build the full description string
(e.g., desc := fmt.Sprintf("SpecGraph spec %s: %s", spec.Slug, spec.Intent)) and
serialize it using a robust serializer (for example strconv.Quote(desc) or
yaml.Marshal(desc) and strip surrounding document markers) before writing into
the builder so all quotes, backslashes and newlines are correctly escaped;
replace the safeIntent variable and the fmt.Fprintf call accordingly.
---
Nitpick comments:
In `@internal/server/convert.go`:
- Around line 707-718: Change injectToolFromProto to return
(storage.InjectToolType, error) instead of silently returning "" for unknown
enum values: keep the existing case mappings (InjectTool_INJECT_TOOL_CLAUDE_CODE
-> storage.InjectToolClaudeCode, INJECT_TOOL_CURSOR -> storage.InjectToolCursor,
INJECT_TOOL_AGENTS_MD -> storage.InjectToolAgentsMD) and in the default branch
return a non-nil error describing the unknown value; update the caller in
sync_handler.go to use the new signature (e.g., tool, err :=
injectToolFromProto(req.Msg.Tool)) and return
connect.NewError(connect.CodeInvalidArgument, err) when err != nil so unknown
proto numeric values are rejected consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79733fa5-3216-4166-9c9f-5fd920c1f8ab
⛔ Files ignored due to path filters (2)
gen/specgraph/v1/specgraphv1connect/sync.connect.gois excluded by!**/gen/**gen/specgraph/v1/sync.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (20)
cmd/specgraph/inject.gocmd/specgraph/serve.gocmd/specgraph/sync.gointernal/inject/inject.gointernal/inject/inject_test.gointernal/server/convert.gointernal/server/sync_handler.gointernal/server/sync_handler_test.gointernal/storage/memgraph/memgraph.gointernal/storage/memgraph/sync.gointernal/storage/memgraph/sync_test.gointernal/storage/sync.gointernal/sync/adapter.gointernal/sync/beads.gointernal/sync/beads_test.gointernal/sync/exec.gointernal/sync/exec_test.gointernal/sync/github.gointernal/sync/github_test.goproto/specgraph/v1/sync.proto
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sync/beads_test.go
- internal/sync/exec_test.go
- internal/sync/beads.go
- internal/sync/github_test.go
- internal/sync/adapter.go
- internal/storage/memgraph/memgraph.go
- cmd/specgraph/inject.go
- internal/storage/memgraph/sync_test.go
Comment on lines
+97
to
+103
| func (m *mockSyncBackend) DeleteSyncMapping(_ context.Context, specSlug string, adapter storage.SyncAdapterType) error { | ||
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
| k := m.key(specSlug, adapter) | ||
| delete(m.mappings, k) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Return ErrSyncMappingNotFound from the mock when nothing is deleted.
This mock always succeeds, so tests can’t exercise the handler’s not-found path and the fixture no longer matches the SyncBackend contract.
🧪 Align the mock with the real backend contract
func (m *mockSyncBackend) DeleteSyncMapping(_ context.Context, specSlug string, adapter storage.SyncAdapterType) error {
m.mu.Lock()
defer m.mu.Unlock()
k := m.key(specSlug, adapter)
+ if _, ok := m.mappings[k]; !ok {
+ return storage.ErrSyncMappingNotFound
+ }
delete(m.mappings, k)
return nil
}As per coding guidelines, "Mock backends must return sentinel errors (e.g., storage.ErrSpecNotFound, storage.ErrDecisionNotFound) rather than fmt.Errorf() when handler code uses errors.Is() checks".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (m *mockSyncBackend) DeleteSyncMapping(_ context.Context, specSlug string, adapter storage.SyncAdapterType) error { | |
| m.mu.Lock() | |
| defer m.mu.Unlock() | |
| k := m.key(specSlug, adapter) | |
| delete(m.mappings, k) | |
| return nil | |
| } | |
| func (m *mockSyncBackend) DeleteSyncMapping(_ context.Context, specSlug string, adapter storage.SyncAdapterType) error { | |
| m.mu.Lock() | |
| defer m.mu.Unlock() | |
| k := m.key(specSlug, adapter) | |
| if _, ok := m.mappings[k]; !ok { | |
| return storage.ErrSyncMappingNotFound | |
| } | |
| delete(m.mappings, k) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/sync_handler_test.go` around lines 97 - 103, The mock
DeleteSyncMapping currently always returns nil; update
mockSyncBackend.DeleteSyncMapping to check if the computed key (via
m.key(specSlug, adapter)) exists in m.mappings under the mutex and, if not
found, return the sentinel error storage.ErrSyncMappingNotFound (instead of nil
or fmt.Errorf), otherwise delete and return nil so tests can exercise the
handler's not-found path; reference mockSyncBackend.DeleteSyncMapping, m.key,
m.mappings, and storage.ErrSyncMappingNotFound.
- Make DeleteSyncMapping idempotent (return nil for non-existent mappings) - Sanitize error messages in syncWithAdapter and Inject handler - Fix path traversal via filepath.Base on spec.Slug in inject - Separate flag variables for beads/github CLI subcommands - ExecRunner: separate stdout/stderr to prevent corruption - Register GitHubAdapter in serve.go - Replace SpecGetter with storage.SpecReader interface - Use cmd.OutOrStdout() instead of os.Stdout in CLI output - Add slug validation in adapter Push methods - Log constitution load errors instead of silently discarding - Replace containsStr/searchStr with strings.Contains - Add syncWithAdapter test coverage (success, error, dry-run, skip) - Add inject nil-spec and path-traversal tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Review ResponseFixed 16 | Deferred 1 | Closed as duplicate/FP 7
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/inject/inject_test.go (1)
192-210: Add coverage for slugs that should be rejected outright.This only exercises the sanitization path.
Injectalso has an explicit error branch for slugs that clean to"",".", or"/"ininternal/inject/inject.go, so one focused test here would lock down that validation too.Example addition
func TestInject_PathTraversal(t *testing.T) { dir := t.TempDir() spec := &storage.Spec{ Slug: "../../etc/passwd", Intent: "malicious", } files, err := inject.Inject(spec, nil, storage.InjectToolClaudeCode, dir) if err != nil { t.Fatalf("Inject returned error: %v", err) } // filepath.Base sanitizes to "passwd", so the file should be safely inside the output dir. if len(files) != 1 { t.Fatalf("expected 1 file, got %d", len(files)) } expected := filepath.Join(dir, ".claude", "specs", "passwd.md") if files[0] != expected { t.Errorf("expected sanitized path %s, got %s", expected, files[0]) } } + +func TestInject_InvalidSlug(t *testing.T) { + dir := t.TempDir() + for name, slug := range map[string]string{ + "empty": "", + "dot": ".", + "root": "/", + } { + t.Run(name, func(t *testing.T) { + _, err := inject.Inject(&storage.Spec{Slug: slug}, nil, storage.InjectToolClaudeCode, dir) + if err == nil { + t.Fatalf("expected error for slug %q", slug) + } + }) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/inject/inject_test.go` around lines 192 - 210, Add a new unit test (e.g. TestInject_RejectEmptyCleanSlug) in internal/inject/inject_test.go that calls inject.Inject (the Inject function in internal/inject/inject.go) with storage.Spec instances whose Slug values clean to "", "." or "/" (examples: "", ".", "/", "./", "../") and assert that Inject returns a non-nil error (and no files) for each case; this locks down the explicit validation branch that rejects slugs that sanitize to empty or root-like values rather than relying only on filepath.Base sanitization exercised by TestInject_PathTraversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/storage/memgraph/sync.go`:
- Around line 191-205: The implementation of Store.DeleteSyncMapping is
currently idempotent but the interface expects ErrSyncMappingNotFound; update
DeleteSyncMapping to match the documented contract by detecting when no mapping
existed and returning ErrSyncMappingNotFound instead of nil: either run a
pre-check MATCH COUNT or inspect the executeQuery result/summary for affected
rows for the pattern matched by `MATCH (s:Spec {slug: $slug})-[r:SYNCED_TO
{adapter: $adapter}]->(e:ExternalRef)` and if zero mappings are found return
ErrSyncMappingNotFound, otherwise proceed to delete and return nil; keep
references to Store, DeleteSyncMapping, executeQuery and ErrSyncMappingNotFound
to locate the code.
---
Nitpick comments:
In `@internal/inject/inject_test.go`:
- Around line 192-210: Add a new unit test (e.g.
TestInject_RejectEmptyCleanSlug) in internal/inject/inject_test.go that calls
inject.Inject (the Inject function in internal/inject/inject.go) with
storage.Spec instances whose Slug values clean to "", "." or "/" (examples: "",
".", "/", "./", "../") and assert that Inject returns a non-nil error (and no
files) for each case; this locks down the explicit validation branch that
rejects slugs that sanitize to empty or root-like values rather than relying
only on filepath.Base sanitization exercised by TestInject_PathTraversal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e6336cf-42c9-43c7-846f-27cd5da3a664
📒 Files selected for processing (11)
cmd/specgraph/serve.gocmd/specgraph/sync.gointernal/inject/inject.gointernal/inject/inject_test.gointernal/server/sync_handler.gointernal/server/sync_handler_test.gointernal/storage/memgraph/sync.gointernal/sync/beads.gointernal/sync/exec.gointernal/sync/github.gointernal/sync/github_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/inject/inject.go
- internal/sync/github_test.go
- internal/server/sync_handler.go
- cmd/specgraph/serve.go
- internal/sync/github.go
- internal/sync/beads.go
- Add slog.WarnContext to all syncWithAdapter error branches for observability - Include orphaned externalID in CreateSyncMapping failure message - Make injectToolFromProto return error instead of empty string - Add configurable output_dir path restriction (SetAllowedOutputRoot) - Thread context.Context through Adapter.Available() interface - Add SyncConfig.GitHubRepo config field, wire through serve.go - Fix DeleteSyncMapping interface doc to match idempotent behavior - Add empty state guard in GitHubAdapter.Pull - Add SyncGitHub, GetSyncStatus invalid adapter, GetSyncMapping error, CreateSyncMapping failure, converter, Push empty slug, Push invalid URL, Pull empty status tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Review ResponseFixed 16 | Deferred 0 | Failed 0 | Duplicate 1
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/server/sync_handler_test.go (1)
98-104:⚠️ Potential issue | 🟡 MinorReturn
ErrSyncMappingNotFoundfrom the mock when nothing is deleted.The mock always returns
nil, so tests can't exercise the handler's not-found path for delete operations. While the handler may not currently useDeleteSyncMapping, maintaining interface contract consistency ensures the mock remains useful as the handler evolves.🧪 Align the mock with the SyncBackend contract
func (m *mockSyncBackend) DeleteSyncMapping(_ context.Context, specSlug string, adapter storage.SyncAdapterType) error { m.mu.Lock() defer m.mu.Unlock() k := m.key(specSlug, adapter) + if _, ok := m.mappings[k]; !ok { + return storage.ErrSyncMappingNotFound + } delete(m.mappings, k) return nil }Based on learnings: "Mock backends must return sentinel errors (e.g.,
storage.ErrSpecNotFound,storage.ErrDecisionNotFound) rather thanfmt.Errorf()when handler code useserrors.Is()checks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/sync_handler_test.go` around lines 98 - 104, The mock DeleteSyncMapping implementation on mockSyncBackend always returns nil; change it to detect when the key (computed by m.key(specSlug, adapter)) was absent and return the sentinel storage.ErrSyncMappingNotFound instead of nil in that case so tests exercising DeleteSyncMapping can hit the handler's not-found path; keep the existing m.mu.Lock()/defer m.mu.Unlock() and deletion logic but check presence before delete and return storage.ErrSyncMappingNotFound when not present.
🧹 Nitpick comments (2)
internal/server/sync_handler.go (1)
206-215: Path traversal check looks correct but has a subtle edge case.The path validation logic correctly prevents traversal outside the allowed root. However, there's a potential edge case with symlinks:
filepath.Absdoesn't resolve symlinks, so a path like/allowed/symlink-to-outsidecould pass validation.Consider using
filepath.EvalSymlinksfor stricter validation if symlink attacks are a concern in your threat model.🛡️ Optional: Resolve symlinks for stricter validation
if h.allowedOutputRoot != "" { - absDir, absErr := filepath.Abs(outputDir) + absDir, absErr := filepath.Abs(outputDir) + if absErr != nil { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("invalid output directory")) + } + // Resolve symlinks to prevent symlink-based traversal + absDir, absErr = filepath.EvalSymlinks(absDir) if absErr != nil { + // Path may not exist yet, fall back to non-symlink-resolved path + absDir, _ = filepath.Abs(outputDir) + } + absRoot, _ := filepath.EvalSymlinks(filepath.Clean(h.allowedOutputRoot)) + if absRoot == "" { + absRoot = filepath.Clean(h.allowedOutputRoot) + } - return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("invalid output directory")) - } - absRoot := filepath.Clean(h.allowedOutputRoot) if !strings.HasPrefix(absDir, absRoot+string(filepath.Separator)) && absDir != absRoot { return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("output_dir must be within the allowed root directory")) } }This is optional depending on your security requirements. The current implementation is sufficient for most use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/sync_handler.go` around lines 206 - 215, The current path check for outputDir uses filepath.Abs (in the block referencing h.allowedOutputRoot and variable absDir) which does not resolve symlinks; update the validation to resolve symlinks (via filepath.EvalSymlinks) before comparing to the cleaned allowed root (h.allowedOutputRoot): call filepath.Abs on outputDir, then call filepath.EvalSymlinks (or EvalSymlinks then Abs) to get the real path, handle and return the error from EvalSymlinks similarly to absErr, normalize both paths with filepath.Clean (and ensure trailing separator logic remains) and perform the same prefix/equality check against absRoot to prevent symlink-based traversal.internal/server/convert.go (1)
707-718: Consider handlingINJECT_TOOL_UNSPECIFIEDexplicitly.The
injectToolFromProtofunction falls through to the error case forINJECT_TOOL_UNSPECIFIED. While the handler validates this before calling the converter, an explicit case would make the behavior clearer and align with the pattern used insyncAdapterFromProto.♻️ Optional: Add explicit UNSPECIFIED handling
func injectToolFromProto(t specv1.InjectTool) (storage.InjectToolType, error) { switch t { + case specv1.InjectTool_INJECT_TOOL_UNSPECIFIED: + return "", fmt.Errorf("inject tool is required") case specv1.InjectTool_INJECT_TOOL_CLAUDE_CODE: return storage.InjectToolClaudeCode, nil case specv1.InjectTool_INJECT_TOOL_CURSOR: return storage.InjectToolCursor, nil case specv1.InjectTool_INJECT_TOOL_AGENTS_MD: return storage.InjectToolAgentsMD, nil default: return "", fmt.Errorf("unknown inject tool: %v", t) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/convert.go` around lines 707 - 718, Update injectToolFromProto to explicitly handle specv1.InjectTool_INJECT_TOOL_UNSPECIFIED: add a case for INJECT_TOOL_UNSPECIFIED that returns the zero-value storage.InjectToolType (e.g., storage.InjectToolType("") or storage.InjectToolUnspecified if that constant exists) and nil error so the behavior is explicit and matches the pattern used in syncAdapterFromProto; keep the existing default error branch for truly unknown values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/server/sync_handler_test.go`:
- Around line 98-104: The mock DeleteSyncMapping implementation on
mockSyncBackend always returns nil; change it to detect when the key (computed
by m.key(specSlug, adapter)) was absent and return the sentinel
storage.ErrSyncMappingNotFound instead of nil in that case so tests exercising
DeleteSyncMapping can hit the handler's not-found path; keep the existing
m.mu.Lock()/defer m.mu.Unlock() and deletion logic but check presence before
delete and return storage.ErrSyncMappingNotFound when not present.
---
Nitpick comments:
In `@internal/server/convert.go`:
- Around line 707-718: Update injectToolFromProto to explicitly handle
specv1.InjectTool_INJECT_TOOL_UNSPECIFIED: add a case for
INJECT_TOOL_UNSPECIFIED that returns the zero-value storage.InjectToolType
(e.g., storage.InjectToolType("") or storage.InjectToolUnspecified if that
constant exists) and nil error so the behavior is explicit and matches the
pattern used in syncAdapterFromProto; keep the existing default error branch for
truly unknown values.
In `@internal/server/sync_handler.go`:
- Around line 206-215: The current path check for outputDir uses filepath.Abs
(in the block referencing h.allowedOutputRoot and variable absDir) which does
not resolve symlinks; update the validation to resolve symlinks (via
filepath.EvalSymlinks) before comparing to the cleaned allowed root
(h.allowedOutputRoot): call filepath.Abs on outputDir, then call
filepath.EvalSymlinks (or EvalSymlinks then Abs) to get the real path, handle
and return the error from EvalSymlinks similarly to absErr, normalize both paths
with filepath.Clean (and ensure trailing separator logic remains) and perform
the same prefix/equality check against absRoot to prevent symlink-based
traversal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 522ea220-bcdb-424f-8371-56b5912aaa33
📒 Files selected for processing (12)
cmd/specgraph/serve.gointernal/config/config.gointernal/server/convert.gointernal/server/convert_test.gointernal/server/sync_handler.gointernal/server/sync_handler_test.gointernal/storage/sync.gointernal/sync/adapter.gointernal/sync/beads.gointernal/sync/beads_test.gointernal/sync/github.gointernal/sync/github_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/storage/sync.go
- cmd/specgraph/serve.go
- internal/sync/beads_test.go
- internal/sync/adapter.go
- internal/sync/github.go
- Make os.Getwd() failure a fatal startup error (spgr-7m7.1) - Add safety comment guard to Cypher query builder (spgr-7m7.11) - Add MkdirAll to writeAgentsMD for directory creation (spgr-7m7.2) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Return error from syncAdapterToProto for unknown adapters (spgr-7m7.10) - Update callers and tests for new signature Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s error (spgr-7m7.10,7m7.12) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Surface constitution unavailability in inject response summary (spgr-7m7.5) - Add auth status check to GitHubAdapter.Available (spgr-7m7.14) - Add SyncStateConflict to convert test coverage (spgr-7m7.18) - Add handler tests: Available error, ListSpecs error, SetAllowedOutputRoot (spgr-7m7.16, 7m7.17, 7m7.19) - Add CLI smoke tests for sync and inject commands (spgr-7m7.15) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename shadowed err variables to closeErr and bootstrapErr. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Review ResponseFixed 15 | Deferred 2 | Closed (not-a-bug) 2
Also fixed pre-existing govet shadow warnings in serve.go. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/storage/memgraph/sync.go (1)
37-48:⚠️ Potential issue | 🔴 CriticalMake sync mapping creation atomic to prevent duplicate edges under concurrency.
The Line 37 pre-check and Line 50 create are separate operations; concurrent requests can both pass the check and create duplicate mappings.
#!/bin/bash # Verify the create path still does "check then create" as two separate executeQuery calls. rg -n -C3 'Check for existing mapping|existingRecords|Create ExternalRef node and SYNCED_TO edge|CreateSyncMapping' internal/storage/memgraph/sync.goAlso applies to: 50-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/memgraph/sync.go` around lines 37 - 48, Replace the separate "check then create" executeQuery calls in CreateSyncMapping with a single atomic Cypher statement executed via s.executeQuery that MATCHes the Spec (by slug) and ExternalRef (by external_id) and then uses a conditional CREATE guarded by WHERE NOT EXISTS((s)-[:SYNCED_TO {adapter:$adapter}]->(e)) to create the SYNCED_TO relationship only when it doesn't already exist; have the query RETURN a row count (or boolean) so the Go code can detect zero creations and return storage.ErrSyncMappingExists, and otherwise return success; update the code around s.executeQuery and the CreateSyncMapping (or similar) function to use this single query and remove the prior pre-check to prevent duplicate edges under concurrency.
🧹 Nitpick comments (2)
internal/sync/github_test.go (1)
140-150: Prefer exact label-token assertions over substring checks.Current
strings.Containsassertions can pass on accidental supersets (e.g.,"approved-old"). Parse tokens and assert exact membership instead.More precise assertion pattern
func TestFormatLabels(t *testing.T) { spec := &storage.Spec{ Stage: storage.SpecStageApproved, Priority: storage.SpecPriorityP0, } labels := formatLabels(spec) - for _, want := range []string{"specgraph", "approved", "p0"} { - if !strings.Contains(labels, want) { - t.Errorf("formatLabels() = %q, missing %q", labels, want) - } - } + got := map[string]struct{}{} + for _, tok := range strings.Split(labels, ",") { + got[strings.TrimSpace(tok)] = struct{}{} + } + for _, want := range []string{"specgraph", "approved", "p0"} { + if _, ok := got[want]; !ok { + t.Errorf("formatLabels() = %q, missing exact label %q", labels, want) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sync/github_test.go` around lines 140 - 150, The test TestFormatLabels uses substring checks; instead parse the returned labels string from formatLabels into discrete tokens (e.g., split by commas/spaces via strings.FieldsFunc) and assert exact membership of the tokens "specgraph", "approved", and "p0" (and optionally assert the token count) so accidental supersets like "approved-old" don't pass; update the checks in TestFormatLabels to build a set/map of tokens and check exact presence of each expected token.internal/server/convert_test.go (1)
210-213: Remove duplicatedrequire.NoErrorassertions.Line 212 and Line 575 repeat the same assertion immediately after Line 211 and Line 574.
♻️ Proposed cleanup
for _, tt := range tests { got, err := edgeTypeToProto(tt.domain) require.NoError(t, err) - require.NoError(t, err) assert.Equal(t, tt.proto, got) } @@ for _, tt := range tests { got, err := syncStateToProto(tt.domain) require.NoError(t, err) - require.NoError(t, err) assert.Equal(t, tt.proto, got) }Also applies to: 573-576
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/convert_test.go` around lines 210 - 213, Two identical require.NoError checks are duplicated around the edgeTypeToProto test assertions; remove the redundant require.NoError so each test case only calls require.NoError(t, err) once before assert.Equal. Locate the calls surrounding edgeTypeToProto in convert_test.go (references: the edgeTypeToProto invocation and the require.NoError/assert.Equal sequence) and delete the extra duplicate require.NoError occurrences (also fix the other identical block later in the file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/sync_handler.go`:
- Around line 80-82: Replace the direct return of the raw adapter error from
adapter.Available(ctx) with a sanitized error created via the project's
stageError helper: instead of returning
connect.NewError(connect.CodeUnavailable, err), call stageError(err,
connect.CodeUnavailable) (or the project's stageError signature) so internal
details are stripped before returning to clients; keep the original availability
check (adapter.Available) and ensure the connect.CodeUnavailable code is
preserved when constructing the sanitized error.
- Around line 207-215: The current validation in the handler
(h.allowedOutputRoot + checking absDir from filepath.Abs(outputDir) and
strings.HasPrefix) is vulnerable to symlink escapes; resolve symlinks for both
the requested directory and the allowed root (use filepath.EvalSymlinks on
outputDir and h.allowedOutputRoot after making them absolute/clean) and then
validate containment (use filepath.Rel or compare the resolved paths with a
HasPrefix + separator check) so that any symlink that points outside the root is
detected and rejected; update the logic around filepath.Abs(outputDir),
filepath.Clean(h.allowedOutputRoot), and the subsequent containment check to use
the resolved paths instead of raw abs paths.
In `@internal/sync/github_test.go`:
- Around line 218-233: The seqMockRunner currently returns responses only by
sequence and does not validate the invoked command or args, so tests won't catch
CLI-contract regressions; update seqMockRunner.Run to inspect the provided
command and args and compare them against an expected sequence stored in the
mock (e.g., augment seqMockRunner with an expected []struct{cmd string; args
[]string} or embed expected in each responses entry), and if the actual cmd/args
do not match the expectation return a descriptive error (instead of the next
response) so tests fail on wrong command name/order; ensure you reference and
update seqMockRunner.responses and the Run method to perform the validation and
produce clear errors like "seqMockRunner: unexpected command" when mismatches
occur.
---
Duplicate comments:
In `@internal/storage/memgraph/sync.go`:
- Around line 37-48: Replace the separate "check then create" executeQuery calls
in CreateSyncMapping with a single atomic Cypher statement executed via
s.executeQuery that MATCHes the Spec (by slug) and ExternalRef (by external_id)
and then uses a conditional CREATE guarded by WHERE NOT EXISTS((s)-[:SYNCED_TO
{adapter:$adapter}]->(e)) to create the SYNCED_TO relationship only when it
doesn't already exist; have the query RETURN a row count (or boolean) so the Go
code can detect zero creations and return storage.ErrSyncMappingExists, and
otherwise return success; update the code around s.executeQuery and the
CreateSyncMapping (or similar) function to use this single query and remove the
prior pre-check to prevent duplicate edges under concurrency.
---
Nitpick comments:
In `@internal/server/convert_test.go`:
- Around line 210-213: Two identical require.NoError checks are duplicated
around the edgeTypeToProto test assertions; remove the redundant require.NoError
so each test case only calls require.NoError(t, err) once before assert.Equal.
Locate the calls surrounding edgeTypeToProto in convert_test.go (references: the
edgeTypeToProto invocation and the require.NoError/assert.Equal sequence) and
delete the extra duplicate require.NoError occurrences (also fix the other
identical block later in the file).
In `@internal/sync/github_test.go`:
- Around line 140-150: The test TestFormatLabels uses substring checks; instead
parse the returned labels string from formatLabels into discrete tokens (e.g.,
split by commas/spaces via strings.FieldsFunc) and assert exact membership of
the tokens "specgraph", "approved", and "p0" (and optionally assert the token
count) so accidental supersets like "approved-old" don't pass; update the checks
in TestFormatLabels to build a set/map of tokens and check exact presence of
each expected token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78a1f610-9989-4ad2-b712-db58dfd9eafa
📒 Files selected for processing (10)
cmd/specgraph/serve.gocmd/specgraph/sync_test.gointernal/inject/inject.gointernal/server/convert.gointernal/server/convert_test.gointernal/server/sync_handler.gointernal/server/sync_handler_test.gointernal/storage/memgraph/sync.gointernal/sync/github.gointernal/sync/github_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sync/github.go
- internal/inject/inject.go
Comment on lines
+218
to
+233
| type seqMockRunner struct { | ||
| responses []struct { | ||
| output []byte | ||
| err error | ||
| } | ||
| callIdx int | ||
| } | ||
|
|
||
| func (m *seqMockRunner) Run(_ context.Context, _ string, _ ...string) ([]byte, error) { | ||
| if m.callIdx >= len(m.responses) { | ||
| return nil, errors.New("seqMockRunner: no more responses") | ||
| } | ||
| r := m.responses[m.callIdx] | ||
| m.callIdx++ | ||
| return r.output, r.err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect gh invocations in adapter:"
rg -nP --type=go '\bRun\(\s*ctx,\s*"gh"\s*,' internal/sync/github.go -C2 || true
echo
echo "Inspect whether tests assert command/args in runner mocks:"
rg -nP --type=go 'type\s+(mockRunner|seqMockRunner)\b|func\s+\(.*\*\s*(mockRunner|seqMockRunner)\)\s+Run\(' internal/sync/github_test.go -C4 || trueRepository: seanb4t/specgraph
Length of output: 1453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== seqMockRunner usage in tests ==="
rg -nP --type=go 'seqMockRunner\{|seqMockRunner\(' internal/sync/github_test.go -A5 -B1
echo ""
echo "=== Test functions using seqMockRunner ==="
rg -nP --type=go 'func Test\w+.*\(' internal/sync/github_test.go | head -20
echo ""
echo "=== Check if any test constructs seqMockRunner and validates command ==="
rg -nP --type=go 'cmd|args' internal/sync/github_test.go -B2 -A2 | grep -A5 -B5 seqMockRunner || trueRepository: seanb4t/specgraph
Length of output: 1327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== TestGitHubAdapter_Available (lines 22-30) ==="
sed -n '22,30p' internal/sync/github_test.go
echo ""
echo "=== TestGitHubAdapter_AvailableNotInstalled (lines 31-42) ==="
sed -n '31,42p' internal/sync/github_test.go
echo ""
echo "=== TestGitHubAdapter_Push (lines 44-61) ==="
sed -n '44,61p' internal/sync/github_test.go
echo ""
echo "=== Look for any error expectations that depend on specific commands ==="
rg -nP --type=go 'errors\.Is|ErrAdapterNotAvailable|ErrPushFailed|ErrPullFailed' internal/sync/github_test.go -B2 -A1 | head -40Repository: seanb4t/specgraph
Length of output: 2735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All tests using seqMockRunner ==="
rg -nP --type=go 'NewGitHubAdapter\(&seqMockRunner' internal/sync/github_test.go -B2 -A15
echo ""
echo "=== Check if mocks distinguish between different gh commands ==="
echo "Looking for tests that would fail if wrong gh subcommand was called..."
rg -nP --type=go 'TestGitHubAdapter_AvailableNotAuthenticated' internal/sync/github_test.go -A30 | head -40Repository: seanb4t/specgraph
Length of output: 1388
seqMockRunner should validate command/args to catch CLI-contract regressions.
The mock returns responses in sequence without checking which gh commands are being invoked. This allows tests to pass even if the adapter calls commands in the wrong order or with incorrect arguments. For example, TestGitHubAdapter_AvailableNotAuthenticated expects gh --version followed by gh auth status, but the mock would return the same responses regardless of the actual command sequence.
Validate both command name and arguments in Run() to ensure adapter changes that violate the CLI contract are caught by tests:
Suggested validation for seqMockRunner
+type seqResponse struct {
+ cmd string
+ args []string
+ output []byte
+ err error
+}
+
type seqMockRunner struct {
- responses []struct {
- output []byte
- err error
- }
+ responses []seqResponse
callIdx int
}
-func (m *seqMockRunner) Run(_ context.Context, _ string, _ ...string) ([]byte, error) {
+func (m *seqMockRunner) Run(_ context.Context, cmd string, args ...string) ([]byte, error) {
if m.callIdx >= len(m.responses) {
return nil, errors.New("seqMockRunner: no more responses")
}
r := m.responses[m.callIdx]
m.callIdx++
+ if r.cmd != "" && cmd != r.cmd {
+ return nil, errors.New("seqMockRunner: unexpected command")
+ }
+ if len(r.args) > 0 && strings.Join(args, "\x00") != strings.Join(r.args, "\x00") {
+ return nil, errors.New("seqMockRunner: unexpected args")
+ }
return r.output, r.err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/sync/github_test.go` around lines 218 - 233, The seqMockRunner
currently returns responses only by sequence and does not validate the invoked
command or args, so tests won't catch CLI-contract regressions; update
seqMockRunner.Run to inspect the provided command and args and compare them
against an expected sequence stored in the mock (e.g., augment seqMockRunner
with an expected []struct{cmd string; args []string} or embed expected in each
responses entry), and if the actual cmd/args do not match the expectation return
a descriptive error (instead of the next response) so tests fail on wrong
command name/order; ensure you reference and update seqMockRunner.responses and
the Run method to perform the validation and produce clear errors like
"seqMockRunner: unexpected command" when mismatches occur.
Log raw adapter error with slog.WarnContext before returning sanitized "sync adapter not available" message to client, consistent with handler error sanitization convention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove dead "/" check from filepath.Base result validation. Add regexp pattern to reject slugs with special characters (newlines, null bytes, spaces). Add test cases for invalid slug special characters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Check spec.Stage.IsValid() and spec.Priority.IsValid() before formatting labels, preventing malformed values from creating unexpected GitHub labels. Add tests for invalid stage and priority. Fix existing tests to include valid Stage/Priority fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…8,.19,.21,.23)
Owner
Author
Review ResponseFixed 22 | Deferred 0 | Failed 0 Production Fixes
Test Coverage Additions
All fixes pass |
Code fixes: - Protect allowedOutputRoot with mutex in SyncHandler (.1) - Use marker-based editing for AGENTS.md to prevent overwrites (.2/.12) - Escape backslashes in Cursor frontmatter intent (.3) - Validate externalID format before CLI calls in Pull methods (.4) - Remove externalID leak from client-visible error message (.5) - Sanitize injectToolFromProto error for clients (.6) - Rename proto field dry_run to dry_run_count for clarity (.7) - Validate gh issue create URL scheme and structure (.8) - Add warnings field to InjectResponse proto (.9) - Add adapter context to GetSyncMapping warning log (.10) - Omit empty stderr suffix from exec error string (.11) - Reorder runInject to validate tool before client creation (.19) Test additions: - GetSyncStatus ListSyncMappings error path (.16) - BeadsAdapter.Push malformed JSON and missing ID (.17) - Inject handler constitution-warning path (.18) - CLI inject tool alias resolution (.19) - CreateSyncMappingExists branch coverage (.21) - GitHub Pull empty/invalid externalID, Push empty/bad-scheme output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Review ResponseFixed 17 | Deferred 3 | Duplicate 1
|
Address 17 review findings across security, reliability, portability, correctness, and test coverage: - Remove internal os.Getwd fallback from RegisterSyncService; callers must call SetAllowedOutputRoot explicitly - Extract advisory file locking into platform-specific files with build constraints (lock_unix.go, lock_windows.go) - Upgrade adapter-unavailable log from Warn to Error (hard failure) - Fix retry path: ErrSyncMappingExists now counts as Skipped - Sanitize unsupported-adapter error to avoid leaking internal enums - Validate github.com host in Push/Pull to prevent SSRF - Change formatLabels to return []string with individual --label args - Use MERGE instead of CREATE for ExternalRef to prevent TOCTOU dupes - Escape pipe chars and newlines in markdown table cells - Add test coverage for retry paths, empty output dir, invalid stage/priority, default timeout, host validation, and empty repo Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Review ResponseFixed 17 | Deferred 0 | Failed 0
All quality gates pass ( |
Owner
Author
Review ResponseFixed 14 | Closed (stale/invalid) 4 | Deferred 1
|
…ilename Handle empty ConstitutionPath (use CWD) and bare filenames (reject with clear error). Prevents double filepath.Dir from resolving to wrong root. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…putDir Move filepath.Abs resolution outside the allowedRoot check so absDir is always available. Pass absDir (not outputDir) to inject.Inject to prevent TOCTOU divergence between path validation and file operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ping After deleting the SYNCED_TO edge, also delete the ExternalRef node if no remaining edges point to it. Prevents graph bloat from orphaned nodes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ect handler Add filepath.EvalSymlinks after filepath.Abs to prevent symlink-based directory escape. Falls back to unresolved path if dir doesn't exist yet. Pass realDir to inject.Inject for consistency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Write to temp file then os.Rename to prevent corruption from partial I/O failures in writeAgentsMD. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Surface non-fatal stderr output (rate-limit warnings, deprecation notices) via slog.Debug instead of silently discarding it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dler Distinguish ErrConstitutionNotFound (Debug, no warning) from other errors (Warn with specific message) to improve operator visibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s, errors, atomic writes)
Owner
Author
Review ResponseFixed 12 | Deferred 0 | Failed 0
|
…ing, test fixes)
Owner
Author
Review Response (Turn 2)Fixed 6 | Deferred 6 | Failed 0
|
…ML newlines, table escaping)
Owner
Author
Review Response (Turn 3)Fixed 4 | Deferred 2 | Failed 0
|
…ification Memgraph raises 'Cannot resolve conflicting transactions' at the transaction layer, bypassing the Cypher WHERE guard path. Now detected in executeQuery and wrapped with ErrConcurrentModification. Also fixes two flaky concurrent modification tests: - TestSupersedeSpec: rewrote with actual goroutine-based concurrency - TestAcknowledgeDrift: tolerate ErrConcurrentModification from races
seanb4t
force-pushed
the
feat/slice-6-sync-integration
branch
from
c056a5d to
125eb49
Compare
…ification Memgraph raises 'Cannot resolve conflicting transactions' at the transaction layer, bypassing the Cypher WHERE guard path. Now detected in executeQuery and wrapped with ErrConcurrentModification. Also fixes: - DeleteSyncMapping Cypher: use OPTIONAL MATCH for orphan cleanup (Memgraph 2.4 doesn't support anonymous node patterns in WHERE) - TestSupersedeSpec: rewrote with actual goroutine-based concurrency - TestAcknowledgeDrift: tolerate ErrConcurrentModification from races
…ification Memgraph raises 'Cannot resolve conflicting transactions' at the transaction layer, bypassing the Cypher WHERE guard path. Now detected in executeQuery and wrapped with ErrConcurrentModification. Also fixes: - DeleteSyncMapping Cypher: use OPTIONAL MATCH for orphan cleanup (Memgraph 2.4 doesn't support anonymous node patterns in WHERE) - TestSupersedeSpec: rewrote with actual goroutine-based concurrency - TestAcknowledgeDrift: tolerate ErrConcurrentModification from races - TestExecution_FullLifecycle: order-agnostic event assertions (ULID ordering within same millisecond is non-deterministic)
Consolidate 9 sync tests into subtests of a single TestSync parent, sharing one Memgraph container. Each subtest clears the graph via DETACH DELETE. Saves ~48s of container startup overhead in CI, preventing the 10-minute test timeout. Also makes TestExecution_FullLifecycle event assertions order-agnostic since ULID ordering within the same millisecond is non-deterministic.
Consolidate lifecycle (32→1), authoring (24→2), and execution (8→1) integration tests into parent test functions with subtests sharing a single Memgraph container. Each subtest clears the graph between runs. Reduces container startups from ~86 to ~18, saving ~408s of Docker overhead in CI and preventing the 10-minute test timeout.
Graph edges use slug-based references. The e2e test incorrectly compared e.ToId against the spec's ULID Id field instead of its Slug.
Memgraph enforces referential integrity — MATCH requires both nodes to exist, so dangling edges cannot be created. The edge.dangling_ref linter rule is covered by unit tests with mock backends.
Memgraph transaction locking can cause the Cypher WHERE guard to return 0 rows even when preconditions match, resulting in ErrInternalGuardFailure. This is a valid concurrent race outcome alongside ErrConcurrentModification.
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
1 participant
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
sync.proto), domain types, storage interfaces, and Memgraph implementation using[:SYNCED_TO]edgesbd/gh) withCommandRunnerabstraction for testabilityinternal/inject/) writing spec context to.claude/specs/,.cursor/rules/, orAGENTS.mdSyncServicehandler with proto↔domain converters and register inserve.gosync beads,sync github,sync status,inject <slug>New packages
internal/sync/internal/inject/internal/storage/memgraph/sync.goTest plan
sync/,inject/,server/packages//go:build integration) for CRUD operationstask checkpasses (fmt, license, lint, build, unit tests)task pr-prep(integration + e2e with Docker)sync --help,sync beads --help,sync github --help,sync status --help,inject --help🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Configuration
Tests