Production-ready synthesis candidate - #402
Open
thebtf wants to merge 126 commits into
Open
Conversation
|
Important Review skippedToo many files! This PR contains 630 files, which is 330 over the limit of 300. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (829)
📒 Files selected for processing (630)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Owner
Author
|
@coderabbitai review focus:behavioral correctness, lifecycle concurrency, security and low-cardinality telemetry, release regressions |
Owner
Author
|
@codex review |
|
Kicking off the review now. ✅ Action performedReview finished.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request establishes the release gates foundation and documents various verification, database, and security evidence reports, including bulk operations and test pool hygiene reworks. The feedback recommends standardizing the package path formats in the critical suite configuration to ensure robust path matching in coverage assertion scripts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+36
to
+44
| "internal/module/": 75 | ||
| "internal/handlers/engramcore": 60 | ||
| "internal/handlers/loom": 70 | ||
| "cmd/engram/": 10 | ||
| "cmd/engram-server/": 10 | ||
| "internal/update/": 20 | ||
| "internal/worker/": 55 | ||
| "internal/mcp/": 55 | ||
| "internal/db/gorm/": 55 |
There was a problem hiding this comment.
There is an inconsistency in the trailing slashes of package paths. For example, "internal/handlers/engramcore" and "internal/handlers/loom" do not have trailing slashes, whereas other packages like "internal/module/" and "cmd/engram/" do. This inconsistency can lead to brittle path matching in the coverage assertion scripts. Standardizing the format by removing trailing slashes across all package keys is recommended.
"internal/module": 75
"internal/handlers/engramcore": 60
"internal/handlers/loom": 70
"cmd/engram": 10
"cmd/engram-server": 10
"internal/update": 20
"internal/worker": 55
"internal/mcp": 55
"internal/db/gorm": 55![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888708b3f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return unavailableProjectIdentity(err) | ||
| } | ||
| canonical := bindingKey | ||
| if len(legacyCandidates) == 1 && projectIsUnboundLegacy(legacyCandidates[0]) { |
There was a problem hiding this comment.
When a v2 registration supplies a selector and legacy_project_id that already resolve to different existing project rows, findCombinedProjectCandidates returns more than one row; this code only handles the len == 1 case, then falls through with canonical = bindingKey and creates/appends a new canonical row for both aliases. That corrupts the project identity map and splits future data under a third project instead of failing as ambiguous before mutation.
Useful? React with 👍 / 👎.
| @@ -613,10 +838,22 @@ async function RunHook(hookName, handler) { | |||
| LegacyProject: LegacyProjectID(cwd), | |||
| GitRemote: gitResult ? gitResult.gitRemote : '', | |||
| RelativePath: gitResult ? gitResult.relativePath : '', | |||
| ProjectIdentityV2: resolveProjectIdentityV2(cwd), | |||
There was a problem hiding this comment.
For non-git workspaces where .engram-project-v2.json cannot be read or created (for example a read-only checkout or a malformed existing anchor), resolveProjectIdentityV2(cwd) throws while building context, before the try block that writes the pass-through response. In that scenario the hook process rejects without calling writeResponse, so all hook types can fail hard instead of preserving the existing offline/pass-through behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5722882efc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if identity.GitRemote != "" && !projectHasGitIdentity(bound[0], identity.GitRemote, identity.RelativePath) { | ||
| return ambiguousProjectIdentity("binding key conflicts with stored git identity") | ||
| } | ||
| if err := appendProjectAliases(ctx, tx, canonical, selector, identity.LegacyProjectID, bindingKey); err != nil { |
There was a problem hiding this comment.
When the full identity binding already resolves, this branch appends selector and legacy_project_id to the bound canonical row without first checking whether either alias already selects a different active project. In a migration case where git identity A is already registered but the incoming legacy selector still has old row B, this adds B's selector as an alias of A, so future legacy resolution returns both rows and the identity map becomes ambiguous/corrupted. Please check the aliases for conflicting candidates before mutating here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cce0832274
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -29,6 +30,10 @@ func (m *Module) ProxyTools(ctx context.Context, p muxcore.ProjectContext) ([]mo | |||
| } | |||
| token := m.envFor(p, config.EnvWorkstationToken) | |||
| project := m.cache.Resolve(p) | |||
| projectIdentity, err := resolveProjectIdentityV2(p.Cwd) | |||
There was a problem hiding this comment.
In non-git workspaces where the checkout is read-only or .engram-project-v2.json is malformed/unwritable, resolveProjectIdentityV2 returns an error before Initialize runs. Because this is in ProxyTools, the dispatcher gets an error and omits the entire dynamic tool list even though the server can still accept the legacy Project selector with a nil ProjectIdentity; this regresses existing read-only/non-git workspaces from usable legacy scoping to no engram tools.
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
|
|
||
| const anchor = readOrCreateProjectAnchorV2(workspaceDir); |
There was a problem hiding this comment.
For non-git OpenClaw workspaces on a read-only directory (or with an unreadable/malformed existing anchor), readOrCreateProjectAnchorV2 throws here before registerAndResolveProject can produce its structured failure. Tool executors such as engram_search and memory_store call resolveIdentity without a surrounding catch, so those requests reject/crash instead of returning the plugin's normal unavailable message or falling back to the legacy project selector.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08987ddb8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const registration = await this.client.registerAndResolveProject(this.identity, this.projectId); | ||
| if (!registration.ok) { | ||
| this.logger.warn(`[file-watcher] project registration failed: ${registration.error.code}`); | ||
| return; |
There was a problem hiding this comment.
When OpenClaw starts while the engram server is temporarily unavailable or project registration returns a retryable 503, this early return exits before chokidar is installed. Since this is the registered service's start() path and syncFile() already handles later client unavailability checks, MEMORY.md/memory changes are never watched or synced until the plugin is reloaded; keep the watcher running and retry registration during sync instead of permanently disabling the service.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8279d22184
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+402
to
+403
| if v, err := envOrFile(EnvDatabaseDSN, EnvDatabaseDSNFile); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
When deployments use only DATABASE_DSN_FILE (the new compose files do), a missing or empty mounted secret makes this branch return an error from Load(), but normal startup goes through config.Get(), which discards Load() errors and installs Default(). The server then proceeds with an empty DSN and reports a generic database initialization failure instead of the configured secret-file error, making a bad production secret mount hard to diagnose; please make file-backed secret errors fatal/logged on startup or avoid swallowing them in the Get() path.
Useful? React with 👍 / 👎.
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.
Full production-readiness synthesis candidate. Latest observability safe point wires real gRPC/auth/version/database/index diagnostics and daemon-only OTLP lifecycle using otelgrpc v0.68.0. Evidence: full go test/vet/build PASS; OTLP smoke PASS; critical suite PASS 197/197 with zero skips; focused count-5, race count-3, and production-mutation Prove-It PASS. Root review is recorded; independent blind review is requested before acceptance. Do not merge until remaining production-ready gates and final release review are complete.