perf(prompt): progressively disclose fresh context - #5077
Conversation
Keep project authority eager while bounding the ambient skills index and retaining complete discovery through load_skill. Move the session relay template onto the relay action instead of every fresh system prompt. Make context diagnostics count the configured prompt rather than a disabled project pack and approximate phantom layers, and pin the lazy boundaries with provider-free tests.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @Hmbown's task in 4m 14s —— View job Review complete
SummaryThis is a well-scoped, mechanically clean progressive-disclosure PR: relay template moved out of the eager system prompt, ambient skills index capped and budget-checked against a hard char ceiling, and Good choices worth calling out:
Minor / non-blocking observations:
Neither observation blocks merge; both are edge cases well outside the scenarios the PR's receipts and test suite exercise. Note: I could not execute |
There was a problem hiding this comment.
🟡 Not ready to approve
The new hard-budget skill index can still exceed its ceiling due to under-reserving omission-line length, and the context report currently mislabels an always-included environment block as per-request.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR reduces fresh-session prompt footprint by progressively disclosing skill context, making the relay template action-local, and tightening doctor --context-json to report what’s actually configured/active.
Changes:
- Cap the ambient skills index to a hard 2,400-character budget while keeping full discovery via
load_skill name="list". - Remove the session-relay template from the default system prompt and inject it only for
/relay. - Make context reporting more faithful (e.g., core execution prompt text is counted directly; project pack/reporting is conditioned; new authority/environment entries).
File summaries
| File | Description |
|---|---|
| crates/tui/src/tools/skill.rs | Updates tool docs to emphasize name="list" progressive discovery. |
| crates/tui/src/skills/tests.rs | Adjusts expectations for the new budgeted/renamed skills block sections. |
| crates/tui/src/skills/mod.rs | Implements the 2,400-char hard ceiling and new “Usage”/omission behavior in the ambient skills block. |
| crates/tui/src/skills/catalog_matrix.rs | Updates catalogue tests to accept progressive subsets and enforce the hard block budget. |
| crates/tui/src/prompts/text.rs | Reworks COMPACT_TEMPLATE into a session-relay template intended for /relay injection only. |
| crates/tui/src/prompts.rs | Removes eager relay-template inclusion from fresh prompts; exposes helpers used by diagnostics. |
| crates/tui/src/core/engine/tool_catalog.rs | Ensures load_skill stays active by default for first-turn discovery. |
| crates/tui/src/core/engine/tests.rs | Updates default-native-tool contract expectations to include load_skill. |
| crates/tui/src/context_report.rs | Refactors prompt source accounting (skills mode/plugins, core execution counted as text, new authority/environment entries, project pack conditioning). |
| crates/tui/src/commands/groups/session/relay.rs | Injects the relay template into the /relay instruction message instead of hardcoding it inline. |
| crates/tui/src/commands/groups/session/acceptance.rs | Updates session-relay acceptance expectations for the injected template headings. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The context report now uses the mode-and-plugin-aware renderers directly. Limit older convenience wrappers to tests or remove them so the production binary remains warning-free under the workspace CI clippy gate.
|
Claude finished @Hmbown's task in 3m 8s —— View job Review complete
SummaryReviewed the diff since my last pass, including the new commit
On the two Copilot-flagged items (re-verified against current
Good choices:
Note on verification: I don't have |
Reserve omission notices from actual catalogue cardinality so the ambient skill index stays inside its runtime character cap even above 9,999 entries. Track project-pack configuration on App for truthful pre-first-turn diagnostics, and label the always-injected environment block consistently.
|
Claude finished @Hmbown's task in 1m 31s —— View job Review complete
SummaryThis synchronize pushed one new commit, Both Copilot findings are now fixed, and correctly so:
Also fixed, beyond what Copilot flagged: my own earlier review noted Reuse/simplification: no new duplication introduced; the fix reuses the existing Note on verification: |
|
Review follow-up on 4a54bbe: besides the two inline Copilot fixes, interactive context diagnostics now retain the configured project-pack flag on App, so a pre-first-turn |
There was a problem hiding this comment.
🟡 Not ready to approve
The /relay instruction currently embeds a template header that can conflict with the requirement to title the artifact # Session relay, and should be made unambiguous before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
crates/tui/src/commands/groups/session/relay.rs:60
- The
/relayinstruction says the artifact should be titled# Session relay, but it inlinesCOMPACT_TEMPLATEstarting with a separate## Session Relay Templateheading. This makes the recommended structure ambiguous (the model may copy the leading H2 into the file, making# Session relayno longer the top title). Consider stripping the template label when embedding it into the instruction message so the first heading in the structure is# Session relay.
let _ = writeln!(out, "Use this relay structure:");
let _ = writeln!(out);
let _ = writeln!(out, "{}", crate::prompts::COMPACT_TEMPLATE.trim());
let _ = writeln!(out);
crates/tui/src/skills/mod.rs:1240
- This doc comment says the helper is for callers like
prompts.rs, but the function is now#[cfg(test)]andprompts.rsno longer calls it. Updating the comment to reflect that it’s test-only will avoid sending readers to a non-existent production call site.
/// Render the system-prompt skills block from every workspace
/// candidate directory plus the global default (#432). Wraps
/// [`discover_in_workspace`] for callers (e.g. `prompts.rs`) that
/// only have the workspace path to hand.
crates/tui/src/commands/groups/session/acceptance.rs:581
- This test currently asserts on the presence of the template label (
## Session Relay Template), which is an internal instruction wrapper rather than the required relay artifact structure. That makes the acceptance test brittle to harmless template/instruction wording changes; it should assert only on the structural headings that matter to the relay output (e.g.# Session relay,## Verification).
assert!(message.contains("# Session relay"));
assert!(message.contains("## Session Relay Template"));
assert!(message.contains("## Verification"));
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Start the action-local relay template at the required artifact title and keep the acceptance contract focused on user-visible structure. Refresh the provider-free runtime receipt for the intentional first-turn load_skill activation while locking in the smaller prompt measurements.
|
Claude finished @Hmbown's task in 1m 33s —— View job Review complete
SummaryThis synchronize pushed one new commit, Both remaining Copilot findings are now fixed, correctly:
Also cleaned up in this commit, beyond what Copilot flagged: the stale doc comment on Verified the skill-budget reservation math holds end-to-end (re-checked since it's the one place a boundary error would silently regress):
Note on verification: |
|
Current-head follow-up on db9e46c: the action-local relay template now starts directly at |
There was a problem hiding this comment.
🟡 Not ready to approve
The updated prompt tests include assertions that don’t actually validate relay-template absence (they check for a substring that never appears), and render_skills_block can still drop warnings entirely when no skills are loaded.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
crates/tui/src/prompts.rs:3108
- Same issue as above: asserting that the prompt does not contain "## Session Relay Template" is ineffective because that substring never appears in COMPACT_TEMPLATE. Use a real template anchor (e.g. "# Session relay") so the test fails if the template is accidentally injected.
// Session goal remains volatile content below the stable static
// layers. The relay template is injected only when relay/compaction
// actually needs it.
assert!(goal_pos > 0);
assert!(!prompt.contains("## Session Relay Template"));
assert!(!prompt.contains("src/lib.rs"));
crates/tui/src/prompts.rs:3585
- This assertion is checking for a substring ("## Session Relay Template") that does not exist in the relay template, so it will never catch accidental reintroduction. Prefer asserting against a real template marker like "# Session relay".
assert!(
execution_pos < handoff_pos,
"## Core Execution must precede the relay block"
);
assert!(!prompt.contains("## Session Relay Template"));
}
crates/tui/src/skills/mod.rs:1342
render_skills_blockreturnsNonewhenregistry.is_empty()(no skills), even ifregistry.warnings()is non-empty. That means a workspace with only discovery warnings (e.g. all skills failed to parse) will hide those warnings from the model-facing prompt block, despite later code rendering a dedicated warnings section when the block is present.
fn render_skills_block(registry: &SkillRegistry, locale: &str, workspace: &Path) -> Option<String> {
if registry.is_empty() {
return None;
}
crates/tui/src/prompts.rs:3074
- The test checks for the absence of "## Session Relay Template", but that string does not exist in the relay template (COMPACT_TEMPLATE starts with "# Session relay"). This assertion can pass even if the relay template is accidentally reintroduced, so it isn't guarding the intended behavior.
This issue also appears in the following locations of the same file:
- line 3103
- line 3580
fn compact_template_is_lazy_in_fresh_prompt() {
let tmp = tempdir().expect("tempdir");
let prompt =
system_prompt_flat_text(&system_prompt_for_mode_with_context(tmp.path(), None));
assert!(!prompt.contains("## Session Relay Template"));
assert!(!prompt.contains("## Verification"));
}
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Keep authority, current user intent, authorization gates, truthfulness, verified completion, mechanism, and exact precedence eager. Move procedural detail to mode/action/skill boundaries, lock first-turn invariants and skill homes with tests, document measured Pi parity, and surface warning-only skill discovery.
|
Claude finished @Hmbown's task in 2m 57s —— View job Review complete
SummaryThis synchronize pushed one new commit, Verified correct:
Worth flagging for the maintainer (design tradeoff, not a bug): Reuse: no new duplication; the omission-reserve and warning-rendering logic in Note on verification: |
|
Pushed Copilot findings fixed:
Kernel review findings fixed:
Measured in
Local verification: prompt 102/102, diagnostic 13/13, skills 91/91, skill-home contract PASS, runtime contract 55/55 exact, fmt/diff checks PASS, workspace all-features warnings-as-errors Clippy PASS, debug build PASS. The release build has not been rerun after the kernel pass; current 7,908/7,844 proof is from the fresh debug binary. Codewhale + |
There was a problem hiding this comment.
🟡 Not ready to approve
The workspace-only skills rendering path does not apply .into_enabled(), which can cause disabled skills to appear in the ambient skills block when skills_dir is None.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
crates/tui/src/skills/mod.rs:1255
render_available_skills_context_for_workspace_with_mode_and_pluginsbuilds the ambient skills block fromdiscover_in_workspace_with_mode_and_plugins(...)without applyingSkillRegistry::into_enabled(). This makes the workspace-only path inconsistent with the workspace+dir variant (which calls.into_enabled()), and can cause disabled skills to be listed in the system prompt wheneverskills_dirisNone(seeprompts.rsmatch arm).
pub fn render_available_skills_context_for_workspace_with_mode_and_plugins(
workspace: &Path,
mode: SkillDiscoveryMode,
locale: &str,
plugins: Option<&crate::plugins::PluginRegistry>,
) -> Option<String> {
let registry = discover_in_workspace_with_mode_and_plugins(workspace, mode, plugins);
render_skills_block(®istry, locale, workspace)
}
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Current-head follow-up ( |
|
Claude finished @Hmbown's task in 1m 6s —— View job Review complete
SummaryThis synchronize pushed one new commit, The fix is correct and minimal ( let registry =
discover_in_workspace_with_mode_and_plugins(workspace, mode, plugins).into_enabled();This makes I also checked Test coverage is adequate: the new Reuse: no duplication — the fix reuses the existing This closes out every issue Copilot raised across all four review rounds on this PR (omission-line budget sizing, environment-block activation label, relay-template heading ambiguity, workless test assertions, warnings-only skill rendering, and now this workspace-only activation filter). Combined with the constitutional-kernel condensation in Note on verification: |
There was a problem hiding this comment.
🟡 Human review recommended
It changes multiple high-leverage prompt assembly and diagnostic surfaces (constitution/kernel, skills disclosure, tool activation, relay injection), so it warrants final human verification of behavioral/regression risk despite strong test coverage.
Review details
Suppressed comments (1)
crates/tui/src/skills/mod.rs:1360
skill_omission_reserveusesregistry.list().len()(which includesExplicitOnlyskills that are intentionally excluded from the ambient index). This over-reserves digits/space and can cause avoidable truncation of user-visible skills under the 2,400-char cap even when all displayable skills would otherwise fit. Reserving based on the count of displayable (non-ExplicitOnly) skills keeps the invariant while maximizing useful entries.
let skill_omission_reserve = format!(
"- ... {} additional skills omitted; call `load_skill` with `name=\"list\"` for the complete catalogue.\n",
registry.list().len()
);
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Current-head follow-up ( |
|
Claude finished @Hmbown's task in 1m 15s —— View job Review complete
SummaryThis synchronize pushed one new commit, The fix is correct ( let model_selectable_skill_count = registry
.list()
.iter()
.filter(|skill| skill.invocation != SkillInvocation::ExplicitOnly)
.count();
let skill_omission_reserve = format!(
"- ... {} additional skills omitted; call `load_skill` with `name=\"list\"` for the complete catalogue.\n",
model_selectable_skill_count
);I traced this against the render loop at This also correctly stops explicit-only "power skills" from stealing budget from the ambient index they're deliberately excluded from displaying in — consistent with the existing Test coverage is solid: Reuse: no duplication introduced; reuses the existing This closes out Copilot's fifth review round. Combined with all prior rounds (omission-line budget sizing, environment-block activation label, relay-template heading ambiguity, ineffective test assertions, warnings-only skill rendering, workspace-only activation filter, and the constitutional-kernel condensation — all previously reviewed with no outstanding issues beyond the disclosed "Restraint" prose tradeoff), I have no further findings on the current head. Note on verification: |
There was a problem hiding this comment.
🟡 Human review recommended
It makes broad, behavior-defining changes to prompt assembly, tool activation, and diagnostics, and should receive a final human review despite strong test coverage.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Final current-head gate receipt for |
Harvested from PR #5238 by @bistack MCP Registry discovery with Registry-first tool selection: registry_sync surfaces the eligible local stdio catalog as a complete model-side candidate set, connect-failure messages classify early-exit/help output and point recovery at the next Registry candidate, and the bundled mcp-discovery skill documents the flow. Conflict resolutions (keep BOTH features where #5077 overlaps): - engine/tests.rs: import union — the PR's merge_new_runtime_mcp_tools alongside main's workspace_write_carve_out_applies. - engine/context.rs: stacked both compaction guards — main's evidence_available pass-through and the PR's registry_sync intact-set preservation. - skills/system.rs: kept main's external tests file; the PR's only new inline tests were the uninstall pair already dropped with #5077 (main's dead-code audit removed uninstall_system_skills). - tools/runtime_mcp.rs: kept both new free functions — main's reject_shell_metacharacters guard and the PR's connect_failure_message. - budgets: source-structure raised for the new 1283-line mcp_registry.rs (175 large modules, 673293 aggregate lines); runtime-contract re-measured exactly at budget (registry tools sit outside the no-mcp default surface the fixture profiles).
Adds #5242 (sub-agent checkpoint resume), #5240 (real shell wait elapsed time), #5234 (#5223 alternate-scroll under mouse capture), #5077 (progressive fresh-context disclosure), and #5238 (MCP Registry discovery); credits @SparkofSpike and @bistack in Contributors. Root and crates/tui CHANGELOGs kept byte-identical in the 0.9.4 section.
…merged surface - tool_category_classifies_the_names_the_registry_actually_registers: load_skill became default-active with the progressive-disclosure kernel (#5077); pin it as "other" (read-only catalogue loader). - contributor_onboarding_ships_at_generation_8_and_keeps_its_refusals: the current catalog generation is 10 (mcp-discovery, #5238).
Summary
AGENTS.md/CLAUDE.mdproject authority eager, matching Pi's host-side discovery modelload_skill name="list"; skill bodies stay lazy/relay; automatic compaction keeps its own structured successor briefdoctor --context-jsonreport the prompt that is actually configured instead of counting a disabled project pack and approximate phantom layersReceipts
Same workspace and conservative characters / 3 estimator throughout:
/Volumes/VIXinSSD/CW/codewhale.doctor --context-jsonsource-entry totalThe 64-token difference between the current doctor and model-facing totals is diagnostic-only accounting: a project-context warning (33) and provider facts (31). Separately transported provider tool schemas are not included in this system-message receipt.
Pi comparison
Installed Pi 0.80.3 was measured through its SDK in the same working directory with its actual default
read,bash,edit, andwriteprompt snippets and extensions disabled.Pi host-discovers and eagerly reads applicable project files; it loaded
/Volumes/VIXinSSD/CW/AGENTS.md(1,635 chars) and/Volumes/VIXinSSD/CW/codewhale/AGENTS.md(13,280 chars). It does not leave discovery to the model. Pi advertises skill metadata eagerly and reads skill bodies on demand.The full configured Codewhale system is 7,844 tokens, 1,380 below the full configured Pi receipt in this environment. With skill indexes removed from both, Codewhale is approximately 7,050 versus Pi's 5,907: the 1,143-token premium is the deliberate constitutional, mode, and runtime-special-feature budget. Codewhale stays smaller overall here because its skill routing index is bounded at 794 tokens; Pi's 16 discovered skill entries added 3,318.
Constitutional boundary
Still eager and test-locked:
Deeper execution cadence, causal debugging, candidate comparison, simplification, verification, and continuity procedures remain available through mode doctrine, bundled on-demand skills, and
/relay. This is not a claim that deleted prose was byte-for-byte relocated. Seedocs/CONSTITUTIONAL_KERNEL_AUDIT.md.Verification
Current head:
cargo fmt --all -- --check: PASSgit diff --check: PASSdeepseek-v4-flash: BLOCKED byResponses API request failed; no review result claimedA release build passed on the earlier 4a54bbe head. It has not been rerun after the constitutional-kernel pass, so packaged/release-binary proof for 7,908 remains UNRUN. The current receipt is from the freshly built debug binary.
Relates #4704.
Relates #4710.
No-Issue: this is a tested partial implementation of #4704 and #4710, but it does not satisfy every umbrella acceptance criterion yet.