mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-08 17:10:50 -10:00
fix(context): render rule and chain-segment source labels repo-relative
Chain-segment ("<!-- scoped instructions: ... -->") and
<project_rule source=...> labels carried absolute paths into the pinned
system prompt, so a checkout move or recase changed the prompt prefix
and could emit a spurious <context_update> history append.
Render both labels repo-relative (git-root-relative, forward slashes),
falling back to workspace-relative for rules outside any checkout and
to the absolute spelling only for paths outside the label root
(unreachable by construction). The label root is computed once and
threaded through context_chain_dirs, so the chain bounds and the
chain-segment labels derive from one and the same git walk.
Regression tests pin repo-relative chain and rule labels, the
workspace-relative fallback without a git root, the absolute escape
hatch for unplaceable paths, and byte-identity of the whole system
block for the same tree checked out at two different locations.
Adapted from the Pinvou fork (Pinvou/CodeWhale 7f04c907d, PR #70);
builds on the file-name labels from the previous commit.
Signed-off-by: asto18089 <[email protected]>
This commit is contained in:
1 parent
6cf3732a88
commit
38e04c5b33
2 files changed
+249
-17
No files matched your search
@@ -28,6 +28,7 @@ use self::pack::generate_bounded_project_overview;
|
||||
pub use self::pack::generate_project_context_pack;
|
||||
pub use self::types::ProjectContext;
|
||||
pub(crate) use self::types::project_instructions_source_label;
|
||||
use self::types::repo_relative_source_label;
|
||||
use self::types::ProjectContextError;
|
||||
|
||||
/// Names of project context files to look for, in priority order.
|
||||
@@ -510,6 +511,19 @@ pub(crate) fn load_project_context_with_imports(
|
||||
// Each rule file is wrapped in a <project_rule> block and appended after
|
||||
// the main instructions content. Security model: same as AGENTS.md —
|
||||
// workspace-contained content only, no absolute-path escape.
|
||||
//
|
||||
// The source label sits inside the pinned system prompt, so it is
|
||||
// rendered repo-relative (forward slashes) and a checkout move or
|
||||
// recase leaves the block byte-identical. `load_rules_from_dir`
|
||||
// only returns paths under `workspace`, so workspace-relative is a
|
||||
// safe fallback spelling when no git root exists; the absolute
|
||||
// spelling remains only for a path outside the root, which is not
|
||||
// reachable by construction. The label root does not depend on the
|
||||
// loop below, so it is computed once up front:
|
||||
// `repo_relative_source_label` must never render a rule before its
|
||||
// root exists, and a `None` sentinel would silently degrade the
|
||||
// label to the absolute spelling.
|
||||
let rules_label_root = find_git_root(workspace).unwrap_or_else(|| workspace.to_path_buf());
|
||||
let mut rules_content = String::new();
|
||||
for rules_dir in rules_dirs_for(imports) {
|
||||
let rules = load_rules_from_dir(workspace, rules_dir);
|
||||
@@ -519,7 +533,7 @@ pub(crate) fn load_project_context_with_imports(
|
||||
}
|
||||
rules_content.push_str(&format!(
|
||||
"<project_rule source=\"{}\">\n{}\n</project_rule>",
|
||||
path.display(),
|
||||
repo_relative_source_label(&path, Some(rules_label_root.as_path())),
|
||||
content.trim()
|
||||
));
|
||||
}
|
||||
@@ -618,13 +632,20 @@ fn load_project_context_with_parents_and_home_imports(
|
||||
imports: &ForeignInstructionImports,
|
||||
) -> ProjectContext {
|
||||
let workspace_canonical = canonicalize_workspace_or_keep(workspace);
|
||||
// Chain-segment labels sit inside the pinned system prompt, so they are
|
||||
// rendered repo-relative: a checkout move or recase leaves the labels —
|
||||
// and therefore the whole prompt prefix — byte-identical. The chain
|
||||
// never leaves the checkout (`context_chain_dirs`), so every labeled
|
||||
// path strips cleanly; without a git root the chain is a single
|
||||
// workspace segment that never emits a label at all.
|
||||
let git_root = find_git_root(&workspace_canonical);
|
||||
let mut ctx = load_project_context_with_imports(&workspace_canonical, imports);
|
||||
|
||||
// Assemble the repository-root → workspace instruction chain. The chain
|
||||
// directories come from Git traversal of the containing checkout, so a
|
||||
// linked worktree contributes its own root and files above the root —
|
||||
// other checkouts, unrelated parents — stay out of scope.
|
||||
let chain_dirs = context_chain_dirs(&workspace_canonical, home_dir);
|
||||
let chain_dirs = context_chain_dirs(&workspace_canonical, git_root.as_deref(), home_dir);
|
||||
// `chain_dirs` is ordered root → workspace; the workspace itself is the
|
||||
// last entry and was already loaded above.
|
||||
let ancestor_dirs = &chain_dirs[..chain_dirs.len().saturating_sub(1)];
|
||||
@@ -649,7 +670,7 @@ fn load_project_context_with_parents_and_home_imports(
|
||||
let mut assembled = String::new();
|
||||
|
||||
for (path, content) in &ancestor_docs {
|
||||
append_chain_segment(&mut assembled, path, content);
|
||||
append_chain_segment(&mut assembled, path, content, git_root.as_deref());
|
||||
}
|
||||
|
||||
// The workspace's own file is the most specific link: it reads last,
|
||||
@@ -660,7 +681,7 @@ fn load_project_context_with_parents_and_home_imports(
|
||||
.source_path
|
||||
.clone()
|
||||
.unwrap_or_else(|| workspace_canonical.clone());
|
||||
append_chain_segment(&mut assembled, &path, &content);
|
||||
append_chain_segment(&mut assembled, &path, &content, git_root.as_deref());
|
||||
} else if let Some((path, _)) = ancestor_docs.last() {
|
||||
// No workspace-level file: the nearest ancestor is the most
|
||||
// specific source.
|
||||
@@ -743,7 +764,8 @@ pub(crate) fn project_context_cache_candidate_paths(
|
||||
// invalidate the cache too. Changing the opt-in set clears the cache
|
||||
// outright (`set_foreign_instruction_imports`), so over-enumerating here
|
||||
// only ever costs an extra reload.
|
||||
for dir in context_chain_dirs(&workspace, home_dir) {
|
||||
let repo_root = find_git_root(&workspace);
|
||||
for dir in context_chain_dirs(&workspace, repo_root.as_deref(), home_dir) {
|
||||
for filename in PROJECT_CONTEXT_FILES {
|
||||
paths.push(dir.join(filename));
|
||||
}
|
||||
@@ -891,12 +913,20 @@ fn is_git_metadata_entry(path: &Path) -> bool {
|
||||
/// repository root down to the workspace (inclusive).
|
||||
///
|
||||
/// Repository identity comes from the containing checkout itself
|
||||
/// ([`find_git_root`]); the chain never crosses the repository boundary, so
|
||||
/// sibling checkouts and unrelated parents stay out of scope. Outside any
|
||||
/// repository only the workspace itself is searched. When `home_dir` is an
|
||||
/// ancestor it remains an outer boundary the walk never leaves.
|
||||
fn context_chain_dirs(workspace: &Path, home_dir: Option<&Path>) -> Vec<PathBuf> {
|
||||
let mut stop = find_git_root(workspace).unwrap_or_else(|| workspace.to_path_buf());
|
||||
/// ([`find_git_root`]), passed in as `repo_root` so the chain bounds and the
|
||||
/// chain-segment labels are derived from one and the same walk; the chain
|
||||
/// never crosses the repository boundary, so sibling checkouts and unrelated
|
||||
/// parents stay out of scope. Outside any repository (`repo_root` is `None`)
|
||||
/// only the workspace itself is searched. When `home_dir` is an ancestor it
|
||||
/// remains an outer boundary the walk never leaves.
|
||||
fn context_chain_dirs(
|
||||
workspace: &Path,
|
||||
repo_root: Option<&Path>,
|
||||
home_dir: Option<&Path>,
|
||||
) -> Vec<PathBuf> {
|
||||
let mut stop = repo_root
|
||||
.map(Path::to_path_buf)
|
||||
.unwrap_or_else(|| workspace.to_path_buf());
|
||||
|
||||
if let Some(home) = home_dir {
|
||||
let home = canonicalize_workspace_or_keep(home);
|
||||
@@ -928,12 +958,20 @@ fn context_chain_dirs(workspace: &Path, home_dir: Option<&Path>) -> Vec<PathBuf>
|
||||
/// The first segment is the file's raw content (a single-file chain stays
|
||||
/// byte-identical to a plain load); every later segment is prefixed with a
|
||||
/// provenance label so the model can tell the scopes apart, wider scopes
|
||||
/// first and the workspace last.
|
||||
fn append_chain_segment(assembled: &mut String, path: &Path, content: &str) {
|
||||
/// first and the workspace last. The label is repo-relative (forward
|
||||
/// slashes) so the pinned system prompt stays stable across checkout moves
|
||||
/// and recasings; filenames alone would collide, since chain segments
|
||||
/// legitimately share the `AGENTS.md` basename.
|
||||
fn append_chain_segment(
|
||||
assembled: &mut String,
|
||||
path: &Path,
|
||||
content: &str,
|
||||
repo_root: Option<&Path>,
|
||||
) {
|
||||
if !assembled.is_empty() {
|
||||
assembled.push_str(&format!(
|
||||
"\n\n<!-- scoped instructions: {} (overrides wider scopes where they conflict) -->\n",
|
||||
path.display()
|
||||
repo_relative_source_label(path, repo_root)
|
||||
));
|
||||
}
|
||||
assembled.push_str(content);
|
||||
@@ -1352,7 +1390,8 @@ pub fn project_instruction_sources(
|
||||
|
||||
// Repository-root → workspace instruction chain. Same dir order the
|
||||
// loader assembles, so wider scopes list first.
|
||||
for dir in context_chain_dirs(&workspace, home_dir) {
|
||||
let repo_root = find_git_root(&workspace);
|
||||
for dir in context_chain_dirs(&workspace, repo_root.as_deref(), home_dir) {
|
||||
let whale = dir.join(DEPRECATED_WHALE_FILENAME);
|
||||
if context_candidate_exists(&whale) {
|
||||
sources.push(InstructionSourceInfo {
|
||||
@@ -1813,8 +1852,8 @@ mod tests {
|
||||
// instructions block byte-identical, so a move emits no spurious
|
||||
// `<context_update>` history append and no absolute path enters a
|
||||
// provider-bound label. Scope note: ancestor-chain and project-rule
|
||||
// labels keep their own (absolute) spellings; relativizing those is
|
||||
// a separate decision.
|
||||
// labels are repo-relative for the same reason; this test pins the
|
||||
// file-name spelling of the workspace-level block.
|
||||
let dir_a = tempdir().expect("tempdir a");
|
||||
let dir_b = tempdir().expect("tempdir b");
|
||||
fs::write(dir_a.path().join("AGENTS.md"), "Pinned content").expect("write a");
|
||||
@@ -1837,6 +1876,176 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn repo_relative_source_label_falls_back_when_path_is_outside_root() {
|
||||
use crate::project_context::repo_relative_source_label;
|
||||
use std::path::Path;
|
||||
// The absolute fallback is an escape hatch for a path the loader
|
||||
// cannot place under the checkout — unreachable by construction —
|
||||
// so it must degrade to a displayable spelling, never panic or
|
||||
// invent a wrong relative label.
|
||||
assert_eq!(
|
||||
repo_relative_source_label(
|
||||
Path::new("/repo/crates/tui/AGENTS.md"),
|
||||
Some(Path::new("/repo"))
|
||||
),
|
||||
"crates/tui/AGENTS.md"
|
||||
);
|
||||
// On unix the backslashes are literal filename bytes, on Windows they
|
||||
// are separators — either way the label must come out forward-slashed,
|
||||
// and this assertion is the only check that can see it off Windows.
|
||||
assert_eq!(
|
||||
repo_relative_source_label(
|
||||
Path::new("/repo/crates\\tui\\AGENTS.md"),
|
||||
Some(Path::new("/repo"))
|
||||
),
|
||||
"crates/tui/AGENTS.md",
|
||||
"backslash separators are normalized to forward slashes"
|
||||
);
|
||||
assert_eq!(
|
||||
repo_relative_source_label(Path::new("/elsewhere/AGENTS.md"), Some(Path::new("/repo"))),
|
||||
"/elsewhere/AGENTS.md",
|
||||
"a path outside the root falls back to the absolute spelling"
|
||||
);
|
||||
assert_eq!(
|
||||
repo_relative_source_label(Path::new("/repo/AGENTS.md"), None),
|
||||
"/repo/AGENTS.md",
|
||||
"a missing root falls back to the absolute spelling"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn chain_segment_labels_are_repo_relative_not_absolute() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let home = tempdir().expect("home tempdir");
|
||||
|
||||
// A two-level chain inside one checkout: the repo root carries the
|
||||
// wide scope, `crates/tui` the workspace scope.
|
||||
let repo = tmp.path().join("repo");
|
||||
fs::create_dir_all(repo.join(".git")).expect("mkdir .git");
|
||||
fs::write(repo.join(".git").join("HEAD"), "ref: refs/heads/main\n").expect("write HEAD");
|
||||
fs::write(repo.join("AGENTS.md"), "ROOT-SCOPE instructions").expect("write root agents");
|
||||
|
||||
let workspace = repo.join("crates").join("tui");
|
||||
fs::create_dir_all(&workspace).expect("mkdir workspace");
|
||||
fs::write(workspace.join("AGENTS.md"), "WORKSPACE-SCOPE instructions")
|
||||
.expect("write workspace agents");
|
||||
|
||||
let ctx = load_project_context_with_parents_and_home(&workspace, Some(home.path()));
|
||||
let instructions = ctx.instructions.as_ref().expect("chain instructions");
|
||||
|
||||
assert!(instructions.contains("ROOT-SCOPE instructions"));
|
||||
assert!(instructions.contains("WORKSPACE-SCOPE instructions"));
|
||||
assert!(
|
||||
instructions.contains("<!-- scoped instructions: crates/tui/AGENTS.md"),
|
||||
"the chain segment label must be repo-relative, got: {instructions}"
|
||||
);
|
||||
assert!(
|
||||
!instructions.contains(&tmp.path().display().to_string()),
|
||||
"absolute paths must not enter chain segment labels: {instructions}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn project_rule_labels_are_repo_relative_not_absolute() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let repo = tmp.path().join("repo");
|
||||
fs::create_dir_all(repo.join(".git")).expect("mkdir .git");
|
||||
fs::write(repo.join(".git").join("HEAD"), "ref: refs/heads/main\n").expect("write HEAD");
|
||||
let rules_dir = repo.join(".codewhale/rules");
|
||||
fs::create_dir_all(&rules_dir).expect("mkdir rules");
|
||||
fs::write(
|
||||
rules_dir.join("security.md"),
|
||||
"# Security\nNo hardcoded secrets.",
|
||||
)
|
||||
.expect("write rule");
|
||||
|
||||
let ctx = load_project_context(&repo);
|
||||
let rules = ctx.rules_block.as_ref().expect("rules block");
|
||||
|
||||
assert!(
|
||||
rules.contains("<project_rule source=\".codewhale/rules/security.md\">"),
|
||||
"the rule label must be repo-relative, got: {rules}"
|
||||
);
|
||||
assert!(
|
||||
!rules.contains(&tmp.path().display().to_string()),
|
||||
"absolute paths must not enter rule source labels: {rules}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prompt_block_is_pinned_across_checkout_moves() {
|
||||
// The whole reason for repo-relative labels: the same repository
|
||||
// tree checked out at two different absolute locations must render
|
||||
// byte-identical system blocks (chain segment + rules included), so
|
||||
// a move or recase neither appends a spurious `<context_update>`
|
||||
// history entry nor invalidates the provider's prompt KV prefix.
|
||||
let mut moved_block: Option<String> = None;
|
||||
for location in ["checkout-a", "checkout-b"] {
|
||||
let root = tempdir().expect(location);
|
||||
let repo = root.path().join("repo");
|
||||
fs::create_dir_all(repo.join(".git")).expect("mkdir .git");
|
||||
fs::write(repo.join(".git").join("HEAD"), "ref: refs/heads/main\n")
|
||||
.expect("write HEAD");
|
||||
fs::write(repo.join("AGENTS.md"), "ROOT-SCOPE instructions").expect("write root");
|
||||
|
||||
let workspace = repo.join("nested");
|
||||
fs::create_dir_all(&workspace).expect("mkdir nested");
|
||||
fs::write(workspace.join("AGENTS.md"), "NESTED-SCOPE instructions")
|
||||
.expect("write nested");
|
||||
// Rules are discovered at workspace level, so the checkout tree
|
||||
// must carry them under the session's workspace too.
|
||||
let rules_dir = workspace.join(".codewhale/rules");
|
||||
fs::create_dir_all(&rules_dir).expect("mkdir rules");
|
||||
fs::write(rules_dir.join("security.md"), "No hardcoded secrets.").expect("write rule");
|
||||
|
||||
let ctx = load_project_context_with_parents_and_home(&workspace, None);
|
||||
let block = ctx.as_system_block().expect("system block");
|
||||
|
||||
assert!(block.contains("<!-- scoped instructions: nested/AGENTS.md"));
|
||||
assert!(
|
||||
block.contains("<project_rule source=\"nested/.codewhale/rules/security.md\">"),
|
||||
"rule label must be repo-relative, got: {block}"
|
||||
);
|
||||
assert!(
|
||||
!block.contains(&root.path().display().to_string()),
|
||||
"absolute paths must not enter the pinned prompt block: {block}"
|
||||
);
|
||||
if location == "checkout-a" {
|
||||
moved_block = Some(block);
|
||||
} else {
|
||||
assert_eq!(
|
||||
moved_block.as_deref(),
|
||||
Some(block.as_str()),
|
||||
"the same tree at two locations must render byte-identical blocks"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn project_rule_labels_fall_back_to_workspace_relative_without_git_root() {
|
||||
// Outside any repository there is no root to relativize against;
|
||||
// the label falls back to workspace-relative spelling, which stays
|
||||
// stable as long as the workspace tree itself is unchanged.
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let rules_dir = tmp.path().join(".codewhale/rules");
|
||||
fs::create_dir_all(&rules_dir).expect("mkdir rules");
|
||||
fs::write(rules_dir.join("security.md"), "No hardcoded secrets.").expect("write rule");
|
||||
|
||||
let ctx = load_project_context(tmp.path());
|
||||
let rules = ctx.rules_block.as_ref().expect("rules block");
|
||||
|
||||
assert!(
|
||||
rules.contains("<project_rule source=\".codewhale/rules/security.md\">"),
|
||||
"the rule label must fall back to workspace-relative, got: {rules}"
|
||||
);
|
||||
assert!(
|
||||
!rules.contains(&tmp.path().display().to_string()),
|
||||
"absolute paths must not enter rule source labels: {rules}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_empty_file_warning() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
|
||||
@@ -134,6 +134,29 @@ pub(crate) fn project_instructions_source_label(source_path: Option<&Path>) -> S
|
||||
.unwrap_or_else(|| "project".to_string())
|
||||
}
|
||||
|
||||
/// The `source` label for prompt blocks that must keep sibling scopes
|
||||
/// distinguishable (ancestor instruction-chain segments, `<project_rule>`
|
||||
/// blocks): the file's path relative to `root` — the containing checkout —
|
||||
/// rendered with forward slashes. When `root` is `None` or `path` is not
|
||||
/// inside it, falls back to the absolute spelling rather than guessing.
|
||||
///
|
||||
/// Like [`project_instructions_source_label`], the label sits inside the
|
||||
/// pinned system prompt, so keeping it stable across directory moves and
|
||||
/// recasings means an unchanged file does not emit a spurious
|
||||
/// `<context_update>` history append after a move, and absolute project
|
||||
/// paths stay out of provider-bound prompt labels. Root-relative spelling
|
||||
/// (rather than the bare file name) is deliberate: chain segments and rule
|
||||
/// files legitimately share basenames across scopes, and the label exists
|
||||
/// precisely to disambiguate them.
|
||||
pub(crate) fn repo_relative_source_label(path: &Path, root: Option<&Path>) -> String {
|
||||
if let Some(root) = root
|
||||
&& let Ok(relative) = path.strip_prefix(root)
|
||||
{
|
||||
return relative.to_string_lossy().replace('\\', "/");
|
||||
}
|
||||
path.display().to_string()
|
||||
}
|
||||
|
||||
/// Merge multiple project contexts (e.g., from nested directories)
|
||||
#[allow(dead_code)] // Public API for monorepo context merging
|
||||
pub fn merge_contexts(contexts: &[ProjectContext]) -> Option<String> {
|
||||
|
||||
Reference in new issue
Block a user