Repository navigation
Expose friendly_name in create_schema/update_schema tool schemas + skill guidance - #2164
Merged
Merged
Conversation
…ill guidance core#2101 added SchemaField.friendly_name (display label, distinct from name and description) but deliberately left packages/agent untouched — the model-facing half of the story. This closes that gap. Tool schemas (packages/agent/src/local_agent/tools.rs): - create_schema and update_schema.add_fields both gain a friendlyName per-field property: the display label, explicitly optional, derived from name when omitted. - Field-level description guidance rewritten from "Field description" (circular, and now actively wrong — description is LLM-facing prose, not a label) to ask for real semantic content: meaning, purpose, usage, an example. - update_schema's top-level description parameter disambiguated from the now-differently-scoped field-level description. rename_fields gains a second operation packages/core didn't yet support: a display-only relabel (from == to, friendlyName set) that updates only the label, migrating no node data — distinct from the identity rename (from != to) that rekeys storage and migrates everything. The two can combine in one entry. Required the underlying mechanism in packages/core/src/schema/mod.rs and a new NodeService::update_schema_field_friendly_name (packages/core/src/services/ node_service/schema.rs) — rename_schema_field's own doc comment already named "update_schema's field-update path" as where this belongs, but no such path existed until now. Skill guidance (packages/agent/src/skill_pipeline.rs, skill_rules.rs): added a RENAME_VS_RELABEL rule teaching which rename_fields shape to use for which user intent — genuinely procedural (which operation, not argument shape, which the tool schema already owns per ADR-064 rule 1). Regenerated packages/skill/SKILL.md via bin/gen_skill_md.rs so the external-agent reference doesn't drift from the same source. Verified rather than assumed: traced exec_create_schema/exec_update_schema confirm they pass args to handle_create_schema/handle_update_schema unmangled (agent-layer end-to-end tests added alongside the existing core-layer coverage), and confirmed the seed-reconciliation content hash (compute_seed_version, packages/core/src/markdown/mod.rs) covers markdown children — already exercised by reseed_replaces_system_tier_node_on_content_change for this exact System-tier skill — so the revised guidance reaches an existing install with no manual version bump. Updated the prompt-assembly snapshot goldens (stage2_candidate_block, stage2_tool_surface) to reflect the new tool-schema and skill-guidance content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
…-path errors
BLOCKER: NodeService::update_schema_field_friendly_name persisted whatever
string it was given verbatim, including an explicit empty or whitespace-only
friendlyName. handle_update_schema's only guard (rename.friendly_name.is_none())
does not catch Some(""), so {"from":"x","to":"x","friendlyName":""} passed
validation and silently wrote an empty label — contradicting
SchemaField::friendly_name's own doc comment that it is always populated,
with every reader assuming so unconditionally.
Fixed by giving update_schema_field_friendly_name the same guard
apply_friendly_name_defaults already applies to an omitted value on
create/add_fields: a blank (trimmed-empty) friendly_name derives one from
the field's name instead of persisting empty, disambiguating against a
sibling field's existing label on collision. Required exposing
disambiguate_friendly_name as pub(crate) so both call sites share one
disambiguation rule rather than two that could drift.
Also: the combined identity-rename + relabel path (one rename_fields entry
with both `to` and `friendlyName` set) runs two sequential, non-atomic
writes — if the first (data migration) succeeds but the second (label
update) fails, the caller needs to know the rename already landed rather
than retrying the original from/to pair, which would now fail differently
since `from` no longer exists. The error message now says so explicitly,
naming the exact retry payload for the label alone.
Test coverage: 3 new tests pin the blank-friendlyName guard (empty string,
whitespace-only, and disambiguation against a colliding sibling label).
test_rename_field_combined_identity_and_friendly_name_rename now creates a
real node and verifies its property data actually migrated to the new key,
not just that the schema definition ended up right. Fixed a stale test
comment claiming to test sibling-field derivation when the payload only
had one field.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 19, 2026
mstomar125
added a commit
that referenced
this pull request
Aug 19, 2026
…ures (#2168) * Trim #2161/#2164 tool-schema bloat; surface turn-level inference failures Measured the actual assembled prompt for the failing ai-chat scenario via NODESPACE_PROMPT_DUMP + the real tokenizer: on this machine the turn's full-tool-surface prompt is 6932 tokens (713 resident prose + 6202 across all 13 fail-open tools + 17 trailer), with create_schema (1341) and update_schema (1034) — the two tools #2164 touched — together accounting for over a third of the tool-schema budget. declare_write_tool_fields contributes zero extra tokens for this scenario (no skill candidate clears the score gate, so field declarations never fire). Trims applied within the tool-schema/skill-guidance channels (ADR-064): - create_schema's per-field name/friendlyName/description descriptions shortened to match update_schema's already-tighter phrasing instead of carrying two different-length explanations of the same argument shape. - update_schema's rename_fields description tightened (identical facts, half the words) — it was restating, almost verbatim, what the RENAME_VS_RELABEL skill rule also says. - RENAME_VS_RELABEL (skill_rules.rs) cut back to the procedural judgment call (which shape the user's intent maps to); the mechanical "from"/"to"/ "friendlyName" explanation now lives only in the tool schema, its correct channel. Regenerated packages/skill/SKILL.md from the trimmed rule. - route_clarify's (#2161) description tightened slightly. - Fixed a pre-existing `cargo clippy --all-targets -D clippy::unused-async` failure in the same file: exec_route_clarify was declared async with no .await in its body (from #2161); made it a plain sync fn. Net: 6932 -> 6810 tokens for the measured scenario (122 saved, confirmed via the real tokenizer, not estimated). The prompt_assembly_snapshot goldens were regenerated to pin the new tool-schema/skill-guidance text. Also fixes the silent-failure half of this issue: local_agent_service.rs's run_ai_chat_turn logged a WARN and reset the node straight to "idle" on any inference failure (including ContextOverflow) with no new assistant message and no way for a polling caller or the frontend to distinguish a failed turn from one still in flight. It now appends an assistant-role message naming the failure and reaching idle through the same path a normal reply uses, matching ADR-062's "refuse loudly, don't clamp silently" principle applied at the per-turn level. Added failed_inference_turn_surfaces_a_visible_error_not_silent_idle as a regression guard. Closes #2167. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address review: restore route_clarify disambiguation clause; document history tradeoff Adversarial review of PR #2168 found no blockers. Two non-blocking items addressed here: - route_clarify's description had dropped "and nothing already said picks one" when trimmed — restored (a few tokens) since without it the text reads as license to clarify on any multi-match search result even when other conversational context already disambiguates. - Documented, at the append_assistant_message call for a failed turn, why feeding the synthesized failure text back into the model's own future history as an ordinary assistant message is accepted rather than filtered/tagged: it can't trip the duplicate-write guard (empty completed_writes), unbounded growth is bounded by the existing maybe_summarize_history mechanism, and a model that sees its own stated failure has exactly the context needed to avoid blindly repeating it. Also: the review noted this PR's first commit message states a net 6932 -> 6810 token result against the issue's reported 5120-token window without reconciling that 6810 > 5120 — worth stating plainly here since that commit's own text doesn't: the 5120 figure is this issue's reported window on ITS reporting machine. On this machine, ADR-062's `fit_n_ctx_to_budget` (packages/nlp-engine/src/chat/mod.rs) grants the full configured 32768-token window given its total RAM, so the trim's 6810-token result already fits comfortably here regardless of the 5120-vs-6810 gap; the previously-failing test now passes for real (the model generates and returns "OK", not a fallback/error path), independently verified via NODESPACE_PROMPT_DUMP + the real tokenizer, not assumed. The per-turn silent-failure fix in the same PR is what makes any machine whose window IS smaller than its own prompt fail visibly instead of silently, regardless of the exact number on that machine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
8 tasks
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.
Closes #2104
Problem
core#2101 added
SchemaField.friendly_name(a display-label field distinct fromname/storage key anddescription/LLM-facing prose) — the core mechanism landed (Rust/TS types, write-boundary defaulting, UI call sites), but the agent-facing half was deliberately deferred:create_schema/update_schema's tool parameter descriptions never mentionfriendlyName, and the per-fielddescriptionguidance still says "Field description" — circular, and now actively wrong oncedescriptionmeans real semantic content rather than a label.What changed
Tool schemas (
packages/agent/src/local_agent/tools.rs)create_schemaandupdate_schema.add_fieldsboth gain afriendlyNameper-field property: the display label, explicitly optional (derived fromnamewhen omitted).descriptionguidance rewritten to ask for real semantic content — meaning, purpose, usage, an example — not a short label.update_schema's top-leveldescriptionparameter (the schema's own description) disambiguated now that field-leveldescriptionmeans something different.A genuine gap in
rename_fields, closed at the core layer it belongs toThe issue's acceptance criteria required
rename_fieldsto distinguish an identity rename (migrates data) from a display rename (does not) — but no such mechanism existed yet inpackages/core.NodeService::rename_schema_field's own doc comment already named "update_schema's field-update path" as where a label-only update belongs, but that path didn't exist. Added it:FieldRenamegains an optionalfriendlyName(camelCase, matching the rest of this wire surface —coreValues, the newfriendlyNameonadd_fields).from == tois now legal exactly whenfriendlyNameis set (a display-only relabel — no node data touched); illegal otherwise (unchanged, still rejected as a no-op).NodeService::update_schema_field_friendly_name(mirrorsrename_schema_field's persistence shape minus the data-migration step) backs the display-only path.This is
packages/core, technically outsidepackages/agent— but it's the specific, minimal mechanism this issue's own acceptance criteria call for, with no existing alternative to expose instead.packages/agent's side is a pure pass-through (exec_create_schema/exec_update_schemaforward args unmangled — verified by tracing, not assumed) and adds no business logic of its own.Skill guidance (
packages/agent/src/skill_pipeline.rs,skill_rules.rs)Added
RENAME_VS_RELABEL, a newSchemaRuleteaching whichrename_fieldsshape to use for which user intent — genuinely procedural ("which operation for this request"), not argument shape (which the tool schema already owns per ADR-064 rule 1, and which this guidance function's own doc comment explicitly keeps out of prose for exactly that reason). Regeneratedpackages/skill/SKILL.mdviabin/gen_skill_md.rsso the external-agent reference doesn't drift from the same source.Verified, not assumed (per the issue's own acceptance criteria)
exec_create_schema/exec_update_schema→ confirmed they pass args tohandle_create_schema/handle_update_schemaunmangled (no agent-layer transformation to worry about).compute_seed_version,packages/core/src/markdown/mod.rs) → confirmed it hashesnode_type/content/propertiesfor every child parsed from a template's markdown, so the revisedschema_creation_guidance()text automatically produces a different hash — no manual version bump needed. Already exercised by the existingreseed_replaces_system_tier_node_on_content_changetest for this exactSeedTier::Systemskill.Testing
packages/core/src/schema/schema_test.rs): display-only relabel updates the label without touching node data,from == towith nofriendlyNameis still rejected, relabeling a nonexistent field fails, combined identity+relabel in one entry.packages/agent/src/local_agent/tools.rs) through the realGraphToolExecutordispatch path (not just the core handler directly): explicitfriendlyNameround-trips, an omitted one is still derived, and the new relabel-onlyrename_fieldsshape works throughupdate_schema. One of these caught a real assumption I would have gotten wrong (derived label is\"Due date\", not\"Due Date\") before I checkedderive_friendly_name's actual output.cargo test -p nodespace-core --lib: 1169 passed (was 1165 baseline for this session).cargo test -p nodespace-agent --lib: 522 passed (was 519).cargo test -p nodespace-agent --test prompt_assembly_snapshot: updated (stage2_candidate_block,stage2_tool_surface— the new tool-schema/guidance content), then re-run clean, 5/5.cargo test -p nodespace-agent --bin gen_skill_md: 3/3, includingchecked_in_skill_md_generated_block_is_up_to_date.cargo clippy --all-targets(both crates): clean.cargo fmt --check: clean for everything in this diff.bun run test:all+cargo build --bin nodespaced+bun run test:e2e+cargo test -p nodespace-app) passed locally before push.🤖 Generated with Claude Code