Improve Rust daemon parity coverage#690
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
The patch introduces security holes in native parity routes, marks ontology proposals as applied without mutating the ontology, removes documented API endpoints used by clients, and reopens a bounded-query performance issue. These are blocking correctness regressions for the Rust daemon parity work. Full review comments:
|
|
Review posted through GitHub's PR review UI for commit |
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
fdd116e8
I found a few concrete parity regressions: the Rust daemon now exposes an ontology apply route that only flips proposal status, and this diff also drops existing API routes while claiming broader route coverage. There is also a migration hardening regression that can let startup continue after required parity columns fail to install.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The issues are directly visible in the changed Rust daemon routes and migration runner: ontology apply updates only ontology_proposals, main.rs removes previously registered API routes, and add_column_if_missing now swallows ALTER failures used by required parity repairs.
| match result { | ||
| Ok(proposal) => (StatusCode::CREATED, Json(proposal)).into_response(), | ||
| Err(error) => ( | ||
| StatusCode::BAD_REQUEST, |
There was a problem hiding this comment.
/api/ontology/proposals/{id}/apply marks a proposal as applied but does not apply the proposal payload to the graph tables. That means clients can observe a successful apply result while entities, entity_attributes, or entity_dependencies remain unchanged. The PR description already calls this a correctness blocker, and the implementation still exposes the mutating success path, so this should not land as native parity until it either performs the graph mutation or refuses apply with a clear non-implemented response.
| let _ = start_synthesis_worker(state.as_ref()).await; | ||
|
|
||
| // Build router | ||
| let app = Router::new() |
There was a problem hiding this comment.
This removes existing Rust daemon routes such as /api/auth/whoami, /api/memories/most-used, /api/embeddings/status, and /api/memory/forget while the PR claims to expand parity route coverage. Unless the TypeScript daemon removed these same public surfaces, this is a breaking API regression for dashboard/CLI clients that already call them. Please keep the compatibility routes or document and test the intentional removal against the TS reference surface.
| }) | ||
| .unwrap_or(false); | ||
|
|
||
| if !has_col { |
There was a problem hiding this comment.
add_column_if_missing now logs and continues on ALTER failures, but ensure_cross_daemon_parity_columns uses it for columns the new Rust routes require. If a table is missing, locked, malformed, or otherwise fails the ALTER, startup still returns Ok(()) and later route queries can fail against the partially repaired schema. The previous fallible helper made required schema repair fail fast; keep that behavior for parity-required columns and reserve best-effort logging for genuinely optional repairs.
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
0730d9aa
I found two correctness/data-integrity issues: this removes existing Rust daemon compatibility routes while adding new ones, and the new file-backed marketplace review writes can lose reviews under concurrent requests.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The route removals are visible in platform/daemon-rs/crates/signet-daemon/src/main.rs and are not mentioned as intentional in the PR description. The marketplace review route uses a read-modify-write JSON file path without locking or serialization, which is directly visible in marketplace_reviews.rs; I did not have the full TypeScript reference implementation in the provided context to compare its exact behavior.
Since my last review:
- Fixed or likely addressed: 3
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| let _ = start_synthesis_worker(state.as_ref()).await; | ||
|
|
||
| // Build router | ||
| let app = Router::new() |
There was a problem hiding this comment.
This PR is framed as expanding Rust daemon parity coverage, but it also removes existing route registrations for /api/auth/whoami, /api/memories/most-used, /api/embeddings/status, and /api/memory/forget. Unless clients have already stopped calling those endpoints, this is a compatibility regression: the Rust daemon will now return 404 for surfaces it previously exposed. If these are intentionally retired, the PR should carry the matching API/contract update; otherwise the route handlers need to stay registered.
| id: uuid::Uuid::new_v4().to_string(), | ||
| target_type, | ||
| target_id, | ||
| display_name, |
There was a problem hiding this comment.
Creating a review does a plain read of reviews.json, inserts into that in-memory vector, then rewrites the whole file. Two concurrent POSTs can both read the same starting file and whichever write lands last drops the other review. This is user-visible data loss for the new marketplace review surface; it needs to go through a serialized write path, a file lock, or the database instead of unsynchronized read-modify-write JSON.
NicholaiVogel
force-pushed
the
ant/rust-daemon-parity-progress
branch
from
0730d9a to
72d565e
Compare
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
72d565e2
I found a breaking API regression in the Rust daemon route table, plus a data-loss race in the new file-backed marketplace review writes.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The router diff explicitly removes previously registered compatibility endpoints while the PR claims broader route parity. The marketplace review handlers also perform read-modify-write JSON file updates without serialization, which is directly visible in the new route implementation.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| let _ = start_synthesis_worker(state.as_ref()).await; | ||
|
|
||
| // Build router | ||
| let app = Router::new() |
There was a problem hiding this comment.
This route table drops existing Rust daemon API surfaces while the PR says it expands parity coverage. /api/auth/whoami, /api/memories/most-used, /api/embeddings/status, and /api/memory/forget are removed from the router here, so clients that already work against the Rust daemon will start getting 404s. If these are intentionally superseded, the PR needs replacement routes or compatibility shims rather than silently shrinking the surface.
| rating, | ||
| title, | ||
| review_body, | ||
| ) |
There was a problem hiding this comment.
Creating reviews uses a plain read-modify-write cycle against reviews.json with no lock or atomic compare/write. Two concurrent POSTs can both read the same file, each insert its own review, and the later write will discard the earlier one. Since this is a mutation endpoint for persisted user data, it should be serialized through a lock/DB transaction or use an atomic file update strategy that prevents lost writes.
NicholaiVogel
force-pushed
the
ant/rust-daemon-parity-progress
branch
from
72d565e to
7ab5afb
Compare
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
7ab5afbc
I found a couple of concrete regressions: this expands a lot of Rust daemon surface area, but it also drops existing compatibility endpoints and adds file-backed marketplace writes that can lose reviews under concurrent requests.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The removed route registrations are explicit in platform/daemon-rs/crates/signet-daemon/src/main.rs, and the marketplace review create path is a read-modify-write against a JSON file with no lock or queued writer. I do not have the full untruncated diff or TS reference handlers here, so I am limiting findings to issues directly visible in the provided changed files.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| let _ = start_synthesis_worker(state.as_ref()).await; | ||
|
|
||
| // Build router | ||
| let app = Router::new() |
There was a problem hiding this comment.
This PR is described as expanding Rust daemon parity coverage, but this change removes several existing Rust API routes: /api/auth/whoami, /api/memories/most-used, /api/embeddings/status, and /api/memory/forget. Unless the TS daemon also removed these endpoints and all clients were migrated, that is a breaking parity regression hidden inside a coverage PR. Please either keep compatibility handlers or document and test the intentional API removal.
| let now = chrono::Utc::now().to_rfc3339(); | ||
| let review = MarketplaceReview { | ||
| id: uuid::Uuid::new_v4().to_string(), | ||
| target_type, |
There was a problem hiding this comment.
Concurrent review submissions can overwrite each other here. create reads the whole reviews.json, inserts one review, then writes the whole file back with no lock, transaction, or single writer. Two POSTs racing through this path will both read the same base list and the later write will drop the earlier review. Since this is user-created marketplace data, it needs serialization around the read-modify-write or storage in the existing DB/write queue.
|
Addressed the current Rust daemon parity review blockers in b93a039.\n\nChanges:\n- Restored the dropped route-table entries: /api/auth/whoami, /api/memories/most-used, /api/embeddings/status, and /api/memory/forget.\n- Added Rust batch-forget preview/execute compatibility for /api/memory/forget, including confirm_token handling for large deletes and if_version rejection matching the TS daemon contract.\n- Added /api/memories/most-used response shape matching TS.\n- Serialized marketplace reviews.json read-modify-write with a process-wide mutex to avoid lost updates under concurrent review submissions.\n\nLocal verification:\n- cargo fmt\n- cargo test -p signet-daemon: 131 passed\n- cargo build -p signet-daemon\n- cargo test -p signet-daemon --test contract_replay -- --ignored --test-threads=1: 35 passed\n- endpoint inventory: all 65 TS-tested daemon endpoints are now represented in Rust route/replay coverage; the four review-blocker routes are present.\n- code-scanning open alerts for refs/pull/690/head before this push: [] |
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
b93a0395
I found two correctness issues in the Rust parity work: a required schema repair path now logs-and-continues on failure, and the new marketplace review writes fail on a fresh workspace because the parent directory is never created.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - Both issues are directly visible in the changed Rust files. The migration helper now swallows ALTER TABLE failures while ensure_cross_daemon_parity_columns still reports success, and the review/config writers call std::fs::write on files under marketplace/ without creating that directory.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| Ok(names.iter().any(|n| n == column)) | ||
| })?; | ||
| }) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
This turns required schema repair into best-effort logging. ensure_cross_daemon_parity_columns now calls add_column_if_missing for columns that the new Rust routes depend on, but if an ALTER fails the migration still returns Ok(()) and the daemon can continue with a partially incompatible DB. That is exactly the kind of parity drift this guard was supposed to prevent; required cross-daemon columns should fail startup/migration if they cannot be installed.
| }; | ||
| // lgtm[rust/path-injection] reviews_path resolves a constant file under the canonical Signet workspace root via workspace_paths::child_file. | ||
| match std::fs::read_to_string(path) { | ||
| Ok(raw) => serde_json::from_str::<Vec<MarketplaceReview>>(&raw).unwrap_or_default(), |
There was a problem hiding this comment.
write_reviews writes <SIGNET_PATH>/marketplace/reviews.json directly, but nothing creates the marketplace directory first. On a fresh workspace, POST /api/marketplace/reviews will return a write error instead of creating the local review ledger the PR says this route provides. write_config has the same problem for reviews-config.json; create the parent directory before writing.
|
Follow-up pushed in Changes:
Validation run in
|
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
359941c9
I found a schema-repair regression that can let the Rust daemon start on an incompatible database after required parity columns fail to install. There are also new file-backed marketplace review writes that fail on a fresh workspace because the parent directory is never created.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The diff explicitly changes required schema repair from a fallible helper to a warning-only helper in platform/daemon-rs/crates/signet-core/src/migrations.rs, then uses that helper for columns the new routes query. The marketplace review create/config paths write files under marketplace/ without creating that directory.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| }) | ||
| .unwrap_or(false); | ||
|
|
||
| if !has_col { |
There was a problem hiding this comment.
This turns required schema repair into best-effort logging. ensure_cross_daemon_parity_columns now calls add_column_if_missing for columns that the new Rust routes immediately depend on, but any ALTER TABLE failure is swallowed and run() still returns Ok(()). That can stamp/start a daemon against a DB still missing settings_json, ontology proposal metadata columns, dependency audit columns, etc., and the failure moves from startup to arbitrary route/runtime errors. The previous fallible helper and test caught exactly this case; required parity repairs should still propagate errors.
| // lgtm[rust/path-injection] reviews_path resolves a constant file under the canonical Signet workspace root via workspace_paths::child_file. | ||
| match std::fs::read_to_string(path) { | ||
| Ok(raw) => serde_json::from_str::<Vec<MarketplaceReview>>(&raw).unwrap_or_default(), | ||
| Err(_) => Vec::new(), |
There was a problem hiding this comment.
write_reviews writes <SIGNET_PATH>/marketplace/reviews.json directly, but nothing creates the marketplace directory first. On a fresh Rust-daemon workspace, POST /api/marketplace/reviews will fail with a filesystem error instead of creating the local review ledger the PR description says this route provides. write_config has the same issue for reviews-config.json; create the parent directory through the workspace path helper before writing.
|
CI is now green for |
|
Addressed the latest review blockers in 66a020d.\n\nFixes:\n- Required cross-daemon parity schema repairs now fail fast again: ensure_cross_daemon_parity_columns propagates add-column/repair failures instead of logging and continuing.\n- Historical per-migration ADD COLUMN compatibility shims remain best-effort, because some older SQL migration files legitimately skip those target tables until later versions. This keeps fresh daemon startup working while preserving fail-fast behavior for runtime-required parity columns.\n- Marketplace reviews/config writes now create the <SIGNET_PATH>/marketplace parent directory before writing reviews.json or reviews-config.json, so fresh workspaces can create local review ledgers.\n\nVerification on current branch before push:\n- cargo fmt\n- cargo test -p signet-core: 43 passed\n- cargo test -p signet-daemon: 131 passed\n- cargo build -p signet-daemon\n- cargo test -p signet-daemon --test contract_replay -- --ignored --test-threads=1: 37 passed\n- cargo test --workspace: passed across core, daemon, mcp-stdio, pipeline, services, shadow, spike/doc-tests\n\nThe two targeted marketplace review replay tests also pass: native roundtrip and concurrent file writes. |
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
66a020dc
I found one data-loss bug in the new file-backed marketplace review routes, plus a smaller validation gap around ratings. The rest of the visible Rust parity additions look consistent with the PR’s draft/progress framing.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_cross_module_context] - The review data overwrite path is directly visible in platform/daemon-rs/crates/signet-daemon/src/routes/marketplace_reviews.rs: read_reviews silently converts parse/read failures into an empty vector, and create then writes that vector back with the new review. I did not have the full untruncated diff for every newly added route, so the pass is strongest on the included changed file contents.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| title, | ||
| review_body, | ||
| ) | ||
| else { |
There was a problem hiding this comment.
This write path can wipe an existing reviews ledger if reviews.json is temporarily unreadable or contains malformed JSON. read_reviews() returns an empty vector on any read/parse failure, then create() inserts the new review and write_reviews() overwrites the file with only that one review. For a local persisted data file, parse errors need to be returned and surfaced before writing, not treated as an empty store.
| std::fs::create_dir_all(parent).map_err(|e| e.to_string())?; | ||
| } | ||
| let raw = serde_json::to_string_pretty(config).map_err(|e| e.to_string())?; | ||
| std::fs::write(path, raw).map_err(|e| e.to_string()) |
There was a problem hiding this comment.
parse_rating rounds before validating the accepted range, so inputs like 0.6 become 1 and 5.4 becomes 5. That weakens the API boundary and can store ratings the caller did not actually submit. Validate the raw value is within 1.0..=5.0 before rounding or require an integer rating.
|
Follow-up pushed in Fixes:
Verification on current branch before push:
|
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
b8d11b12
I found one blocking parity/config regression: the Rust manifest parser no longer accepts the documented predictorPipeline block even though the field and defaults still exist in this module. That can make existing valid agent manifests fall back or fail in paths this PR is trying to harden.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The diff directly removes predictor_pipeline from PipelineV2Config while leaving PredictorPipelineConfig defined, which is strong evidence of a compatibility regression. I could not inspect the TypeScript manifest schema or existing fixture manifests in this environment, so confidence is medium rather than high.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| pub significance: Option<SignificanceConfig>, | ||
| pub predictor: Option<PredictorConfig>, | ||
| pub predictor_pipeline: PredictorPipelineConfig, | ||
| // pub predictor_pipeline: PredictorPipelineConfig, // hard-deprecated in 0.112.0 |
There was a problem hiding this comment.
Dropping predictor_pipeline from the deserialized config shape breaks Rust parity for existing manifests that still contain memory.pipelineV2.predictorPipeline. Because this struct is the Rust daemon's agent.yaml contract, removing the field means the Rust runtime no longer round-trips or exposes that config even though PredictorPipelineConfig still exists below. If this is deprecated, keep the field as an ignored/defaulted compatibility field or add an explicit migration/normalization path; otherwise existing configs silently diverge from the TypeScript daemon surface.
|
Follow-up pushed in Fixes:
Verification on current branch before push:
|
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
6f3cc930
I found one data-integrity problem in the new Rust marketplace review surface: it writes the shared reviews ledger non-atomically, which can leave the TS/Rust compatibility file corrupted or empty.
Confidence: Medium [sufficient_diff_evidence, missing_runtime_repro] - The new marketplace review implementation is fully visible in platform/daemon-rs/crates/signet-daemon/src/routes/marketplace_reviews.rs. I did not run a concurrent/crash repro, but the write path uses a direct std::fs::write to the shared JSON file that the PR explicitly says should survive switching between daemon runtimes.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
|
|
||
| fn write_reviews(state: &AppState, reviews: &[MarketplaceReview]) -> Result<(), String> { | ||
| let path = reviews_path(state).map_err(|e| e.to_string())?; | ||
| if let Some(parent) = path.parent() { |
There was a problem hiding this comment.
This persists the shared marketplace review ledger with a direct std::fs::write. If the daemon crashes mid-write, the filesystem fills, or another runtime/process reads while the write is in progress, reviews.json can be left truncated or temporarily invalid. That undercuts the stated TS/Rust compatibility contract for preserving review data across runtimes. Please write to a temp file in the same directory, fsync if this data is meant to be durable, then atomically rename it into place; the same pattern should cover the config writer too.
|
Follow-up pushed in Fixes:
Verification on current branch before push:
|
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
6f7b6499
I found two issues worth fixing before this parity slice goes further: the new knowledge expansion route can return memories outside the requested agent scope, and the review sync config accepts arbitrary endpoints without validating the URL boundary.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The scoping issue is directly visible in the new /api/knowledge/expand query: it resolves the entity by agent_id, then joins memories without constraining m.agent_id or visibility. The endpoint-url concern is visible in marketplace_reviews.rs, but I do not have the later sync implementation in the provided diff, so the impact depends on how that stored URL is consumed.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 2
- Suppressed due to human rebuttal/out-of-scope: 0
| "SELECT m.id, m.content FROM memory_entity_mentions mem | ||
| JOIN memories m ON m.id = mem.memory_id | ||
| WHERE mem.entity_id = ?1 AND COALESCE(m.is_deleted, 0) = 0 | ||
| ORDER BY m.importance DESC, m.created_at DESC LIMIT 50", |
There was a problem hiding this comment.
This memory lookup drops the caller's agent scope. The route resolves the entity with WHERE agent_id = ?1, but the subsequent memory_entity_mentions -> memories join only filters by entity_id and deletion state. If mention rows or memories are ever shared, imported, repaired, or left stale across agents, /api/knowledge/expand can include another agent's memory content in the response. Please constrain the joined memories to the same agent_id and apply the usual visibility/scope rules for this read path.
| "pending": pending, | ||
| })) | ||
| } | ||
|
|
There was a problem hiding this comment.
The sync config route persists any trimmed endpointUrl value without validating scheme, host, or whether custom endpoints are even allowed. Since this config is for review sync and the stored reviews include local user-authored content, a later sync worker could be turned into an SSRF/data-exfiltration path by setting this to an internal URL. At minimum, reject non-HTTPS URLs and either pin to the Signet reviews host or require an explicit local/dev mode before accepting arbitrary hosts.
|
Addressed the latest review in Fixes:
Verification after this commit:
|
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
e4b03977
I found one migration/versioning issue that breaks the new Rust-only tables on existing TypeScript-created databases, which is exactly the parity path this PR is trying to cover.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The new Rust migrations are registered as versions 40 and 41, while the telemetry SQL itself says it is matching the TypeScript daemon's migration 066. Because the migration runner skips any version already present in schema_migrations, a DB previously migrated by the TypeScript daemon past v41 will not create memory_search_telemetry or ontology_proposals before the new Rust routes query them.
Since my last review:
- Fixed or likely addressed: 2
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| Migration { | ||
| version: 39, | ||
| name: "task-agent-scope", | ||
| sql: include_str!("sql/039-task-agent-scope.sql"), |
There was a problem hiding this comment.
These new Rust migrations use schema versions 40 and 41, but the shared DB may already have those version numbers stamped by the TypeScript daemon. The new SQL even notes that memory-search telemetry corresponds to TS migration 066. On a TS-created database that has already applied versions 40/41, Rust will skip these entries and never create memory_search_telemetry or ontology_proposals, so the new telemetry and ontology routes will fail at runtime with missing-table errors. This needs a versioning strategy that cannot collide with the TS migration namespace, or an explicit required table repair for these new tables during startup.
|
Fixed the latest migration/versioning review in The Rust migration runner now reconciles the Rust-only parity tables after normal migration replay, even when a TS-created DB has already stamped versions 40/41 and would otherwise skip the embedded Rust SQL. Added a regression test that runs migrations, drops Verification after this commit:
|
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
ce0a696b
I found one data-migration bug that can drop existing connector settings from the Rust daemon's visible API surface. The rest of the reviewed diff looks consistent with the draft parity direction, but this one is worth fixing before the migration ships.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The migration logic is directly visible in platform/daemon-rs/crates/signet-core/src/migrations.rs and the connector list route now reads settings_json in platform/daemon-rs/crates/signet-daemon/src/routes/connectors.rs. I did not have the full TypeScript connector route/client contract in the prompt, so the impact is inferred from the changed Rust route and the migration added here.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| add_column_if_missing(conn, "entity_dependencies", "source_path", "TEXT")?; | ||
| add_column_if_missing(conn, "entity_dependencies", "source_root", "TEXT")?; | ||
| add_column_if_missing( | ||
| conn, |
There was a problem hiding this comment.
This backfill preserves the freshly-added default '{}' instead of copying existing config_json for TS-created connector rows. settings_json is added above with TEXT NOT NULL DEFAULT '{}', so existing rows will already have '{}'; COALESCE(NULLIF(settings_json, ''), NULLIF(config_json, ''), '{}') therefore never reaches config_json. Since the Rust connector list route now reads settings_json and no longer returns config_json, existing connector settings can disappear from the Rust API after migration. Treat '{}' as empty for this backfill or only add the default after copying from config_json.
|
Fixed connector settings backfill in
Verification:
|
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
58678043
I found one parity break in the Rust connectors route: the implementation now drops fields that the previous TS-compatible contract exposed. That conflicts with the PR's parity claim and is likely to break existing clients that still read the connector ledger shape.
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The diff removes the connector route test that asserted the TS-style response and changes /api/connectors to select and emit only displayName/settings/enabled-style fields. I could prove the response-shape regression from the provided diff, but the prompt did not include the dashboard/client callers that would show every affected consumer.
Since my last review:
- Fixed or likely addressed: 3
- Still blocking: 0
- Newly found: 1
- Suppressed due to human rebuttal/out-of-scope: 0
| let rows: Vec<serde_json::Value> = stmt | ||
| .query_map([], |r| { | ||
| let settings_str: String = r.get::<_, String>(3).unwrap_or_else(|_| "{}".into()); | ||
| let settings = parse_connector_settings(&settings_str); |
There was a problem hiding this comment.
This changes /api/connectors from the previous TS-compatible response shape to a new camelCase/settings-only shape, dropping fields like config_json, cursor_json, last_sync_at, and last_error. The removed test below was explicitly asserting that older shape, and the PR description claims compatibility/parity for source routes and connector settings backfill. Even if the new dashboard wants settings, this route should keep the existing fields or add the new fields alongside them so Rust does not break clients that switch between TS and Rust daemons.
NicholaiVogel
force-pushed
the
ant/rust-daemon-parity-progress
branch
from
5867804 to
669bf55
Compare
PR-Reviewer-Ant
left a comment
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Review metadata
- Reviewer: pr-reviewer
- Model:
gpt-5.5 - Commit:
669bf55e
I did not find a concrete correctness, security, data-integrity, or breaking-change issue in the provided diff. The visible implementation matches the draft parity scope and includes the claimed literal handling for /api/knowledge/expand/session.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_cross_module_context] - The prompt includes targeted changed code for the migration repairs, knowledge expansion fix, marketplace review persistence, connector backfill, and route wiring, which was enough to check the main claims. The local workspace only contains the review prompt rather than a git checkout, and the ontology diff is truncated after apply_create_link, so I could not independently inspect every later lifecycle handler or run the Rust contract tests.
Since my last review:
- Fixed or likely addressed: 1
- Still blocking: 0
- Newly found: 0
- Suppressed due to human rebuttal/out-of-scope: 0
NicholaiVogel
changed the title
NicholaiVogel
dismissed
PR-Reviewer-Ant’s stale review
Stale blocking review addressed by c7b438e and subsequent commits; latest reviewer pass reports Still blocking: 0.
|
Update after the latest review/CI pass:
Latest local verification:
GitHub CI is green on latest head |
3 participants
Summary
contract_replaywith seeded/body-level assertions for native parity surfaces and broader endpoint coverageLatest update
c7b438ed7fixes the reviewed/api/knowledge/expand/sessionwildcard-matching bug: entity names are treated literally instead of being interpolated as raw SQLLIKEpatterns.session_summary_memories -> memory_entity_mentionslinks first and only using escaped text fallback for longer entity names.%and proves it does not match unrelated summaries via SQL wildcard expansion.Current verification
Passing locally in
/home/nicholai/signet/.worktrees/rust-daemon-parity-progress:cd platform/daemon-rs && cargo fmtcd platform/daemon-rs && cargo check -p signet-daemon -p signet-mcp-stdiocd platform/daemon-rs && cargo test -p signet-daemon— 131/131 passedcd platform/daemon-rs && cargo build -p signet-daemoncd platform/daemon-rs && cargo test -p signet-daemon --test contract_replay knowledge_expand_native_graph_data -- --ignored --test-threads=1— first failed RED with%returningtotal: 1, then passed after the fixcd platform/daemon-rs && cargo test -p signet-daemon --test contract_replay -- --ignored --test-threads=1— 42/42 passedcd platform/daemon-rs && cargo test --workspace— passed across core, daemon, mcp-stdio, pipeline, services, shadow, spike/doc-testsRemaining caveats
This remains a draft/progress PR, not a final 100% parity/cutover claim.
PR Readiness (MANDATORY)
INDEX.md+dependencies.yaml)Migration Notes
Rust daemon parity table repairs are idempotent and avoid deleting shared TypeScript migration ledger entries. The PR does not switch Signet's default daemon runtime.