feat(brownfield): discovery/init/refine with review fixes - #117
Merged
Conversation
…extraction Add the brownfield discovery layer that works without an existing blueprint. Ported from experiment/openspec-swarm branch and adapted to use CairnError, reuse path_derived_id from heuristics, and wire into the CLI. New files: - src/brownfield/discovery.rs: filesystem traversal finding module candidates (MIN_FILES=3, MAX_DEPTH=4, extensions: rs/ts/js/py/go) - src/brownfield/init.rs: cairn init --from-code handler, writes to openspec/changes/brownfield-init/ with --force support - src/brownfield/refine.rs: cairn refine handler, writes to timestamped change directories Updated: - src/brownfield/mod.rs: shared types (write_change, stub_contract, blueprint_delta) and new module declarations - src/cli/mod.rs: wired --from-code flag on init, added refine command - tests/phase_9_brownfield.rs: 17 passing tests replacing 9 cflx_planned stubs with real fixture-based assertions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Sibling edges are now bidirectional: infer_edges adds both A->B and B->A edges for candidates sharing a parent directory. 2. Confidence scale normalized to [0.0, 1.0] to match SuggestedEdgeEntry.confidence contract: >=5 files -> 1.0, >=3 files -> 0.7, <3 files -> 0.3. 3. Refine timestamp collision prevented: unique_change_id appends a counter suffix when the change directory already exists. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6 tasks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRepository brownfield discovery identifies source directories as candidates, computes candidate metadata and sibling edges, and outputs change proposals. New CLI commands ChangesBrownfield discovery to CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/phase_9_brownfield.rs (1)
207-209: ⚡ Quick winRemove fixed sleep and validate rapid successive refine calls.
Line 208 adds a 1-second delay, which slows the suite and avoids directly testing the collision guard on back-to-back calls. Since refine IDs are intended to be unique on rapid calls, this test should run without sleeping.
Suggested patch
- // Wait one second so the timestamp differs. - std::thread::sleep(std::time::Duration::from_secs(1)); let second = bf_refine::run_refine(&root).unwrap();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phase_9_brownfield.rs` around lines 207 - 209, Remove the fixed 1-second sleep and make the test call bf_refine::run_refine(&root) back-to-back to validate rapid successive refine behavior; specifically, delete the std::thread::sleep line and immediately call bf_refine::run_refine(&root) a second time (the existing `second` assignment), then assert the expected outcome (e.g., both calls succeed and produced refine IDs are unique or the collision guard behavior is triggered) to verify that run_refine handles rapid consecutive invocations correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/brownfield/discovery.rs`:
- Around line 98-101: The evidence vector is being built from absolute paths
(files.iter() -> to_string_lossy) while candidate.path is produced via
strip_prefix(root), causing inconsistency; modify the code that builds evidence
(the `evidence: Vec<String>` creation) to produce paths relative to the same
`root` used for `candidate.path` (e.g., call strip_prefix(root) or otherwise
compute a relative path for each `p` in `files`) so all entries in `evidence`
match the relative form, and if needed pass `root` into the scope where `files`
is iterated (or adjust where `dir_counts`/evidence are collected) to ensure
consistency across serialization/comparison.
In `@src/brownfield/refine.rs`:
- Around line 23-24: The current flow calls unique_change_id(root, ×tamp())
then write_change(...), which races: unique_change_id only checks existence and
write_change uses create_dir_all, so two processes can pick the same ID; instead
move the atomic directory reservation into unique_change_id by attempting to
create the directory (use std::fs::create_dir) inside its retry loop and treat
AlreadyExists as a collision (increment counter and retry), returning the
reserved change_id only after a successful create_dir; remove/replace the
create_dir_all usage in write_change (or have write_change assume the directory
was already created) so directory creation and ID selection are atomic and
ownership is guaranteed.
In `@src/cli/mod.rs`:
- Around line 92-99: The CLI dispatch handles parsed.command == "refine" but the
command isn't registered in the CLI help/command catalog; update the CLI command
metadata (the commands/help registry used to generate --help and unknown-command
listings) to add a "refine" entry with a short description and usage so it
appears in help output and unknown-command suggestions; ensure the same command
name matches the dispatch check (parsed.command == "refine") and reference the
existing handler crate::brownfield::refine::run_refine when writing the help
text so maintainers can correlate the help entry with the implementation.
- Around line 83-94: The code is hardcoding Path::new(".") when calling
run_init_from_code and run_refine, which ignores the CLI --file root; update
those calls to use the parsed file/root instead (e.g., Path::new(&parsed.file)
or Path::new(&parsed.root) depending on your CLI struct) so brownfield writes go
to the requested project root, and also pass the same parsed.file/root into
init_project instead of Path::new("."); handle the case where parsed.file may be
an Option by falling back to "." if absent.
In `@tests/phase_9_brownfield.rs`:
- Line 165: The fixture in tests/phase_9_brownfield.rs sets the field confidence
to 2.0 which violates the normalized [0.0, 1.0] contract; update the confidence
value on that fixture (the confidence field used in the brownfield extraction
test) to a normalized value within the 0.0–1.0 range (for example 0.2 or 1.0 as
appropriate for the test) and scan other fixtures in the same test module for
any other out-of-range confidence fields to correct them as well.
---
Nitpick comments:
In `@tests/phase_9_brownfield.rs`:
- Around line 207-209: Remove the fixed 1-second sleep and make the test call
bf_refine::run_refine(&root) back-to-back to validate rapid successive refine
behavior; specifically, delete the std::thread::sleep line and immediately call
bf_refine::run_refine(&root) a second time (the existing `second` assignment),
then assert the expected outcome (e.g., both calls succeed and produced refine
IDs are unique or the collision guard behavior is triggered) to verify that
run_refine handles rapid consecutive invocations correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e82bf3ee-456e-410f-aa2e-4a4d93d8eb6f
📒 Files selected for processing (6)
src/brownfield/discovery.rssrc/brownfield/init.rssrc/brownfield/mod.rssrc/brownfield/refine.rssrc/cli/mod.rstests/phase_9_brownfield.rs
Comment on lines
+23
to
+24
| let change_id = unique_change_id(root, ×tamp()); | ||
| write_change(root, &change_id, &extraction)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: confirm the non-atomic exists-check -> write flow.
rg -n -C3 'run_refine|unique_change_id|exists\(|write_change\(' src/brownfield/refine.rs
rg -n -C2 'create_dir_all|std::fs::write' src/brownfield/mod.rsRepository: cairn-framework/cairn
Length of output: 1513
🏁 Script executed:
rg -n -A 20 'fn write_change' src/brownfield/refine.rsRepository: cairn-framework/cairn
Length of output: 47
🏁 Script executed:
rg -n 'fn write_change' src/brownfield/Repository: cairn-framework/cairn
Length of output: 111
🏁 Script executed:
sed -n '45,80p' src/brownfield/mod.rsRepository: cairn-framework/cairn
Length of output: 1315
Make change-ID reservation atomic before writing artifacts.
Lines 23–24 implement a non-atomic two-phase operation: unique_change_id checks path existence with .exists(), then write_change creates the directory later via create_dir_all. This creates a race window where two concurrent cairn refine runs can both select the same ID before either creates its directory. Since create_dir_all succeeds silently on existing directories, the second run overwrites the first run's artifacts.
Move atomic directory creation into unique_change_id so ID selection and directory ownership happen together. Replace the existence checks with create_dir attempts in a retry loop, catching AlreadyExists errors to increment the counter.
Proposed fix (reserve directory with create_dir in the retry loop)
pub fn run_refine(root: &Path) -> Result<String, CairnError> {
let extraction = super::discovery::discover(root)?;
- let change_id = unique_change_id(root, ×tamp());
+ let change_id = reserve_unique_change_id(root, ×tamp())?;
write_change(root, &change_id, &extraction)?;
Ok(change_id)
}
@@
-fn unique_change_id(root: &Path, ts: &str) -> String {
+fn reserve_unique_change_id(root: &Path, ts: &str) -> Result<String, CairnError> {
let base = format!("brownfield-refine-{ts}");
let changes_dir = root.join("openspec/changes");
- if !changes_dir.join(&base).exists() {
- return base;
- }
- let mut counter = 1u32;
+ std::fs::create_dir_all(&changes_dir).map_err(|e| CairnError::ChangeDiscovery {
+ path: changes_dir.to_string_lossy().to_string(),
+ detail: e.to_string(),
+ })?;
+ let mut counter = 0u32;
loop {
- let candidate = format!("{base}-{counter}");
- if !changes_dir.join(&candidate).exists() {
- return candidate;
+ let candidate = if counter == 0 {
+ base.clone()
+ } else {
+ format!("{base}-{counter}")
+ };
+ match std::fs::create_dir(changes_dir.join(&candidate)) {
+ Ok(()) => return Ok(candidate),
+ Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {
+ counter = counter.saturating_add(1);
+ }
+ Err(e) => {
+ return Err(CairnError::ChangeDiscovery {
+ path: changes_dir.join(&candidate).to_string_lossy().to_string(),
+ detail: e.to_string(),
+ })
+ }
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/brownfield/refine.rs` around lines 23 - 24, The current flow calls
unique_change_id(root, ×tamp()) then write_change(...), which races:
unique_change_id only checks existence and write_change uses create_dir_all, so
two processes can pick the same ID; instead move the atomic directory
reservation into unique_change_id by attempting to create the directory (use
std::fs::create_dir) inside its retry loop and treat AlreadyExists as a
collision (increment counter and retry), returning the reserved change_id only
after a successful create_dir; remove/replace the create_dir_all usage in
write_change (or have write_change assume the directory was already created) so
directory creation and ID selection are atomic and ownership is guaranteed.
There was a problem hiding this comment.
Pull request overview
Adds a brownfield extraction path for bootstrapping/refining projects from existing source trees, wiring new discovery/init/refine modules into the CLI and replacing several planned Phase 9 tests with executable coverage.
Changes:
- Adds filesystem-based brownfield candidate discovery and artifact generation.
- Adds
cairn init --from-codeandcairn refineexecution paths. - Expands Phase 9 tests for discovery, init, refine, and heuristic thresholds.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
src/brownfield/discovery.rs |
Adds source directory traversal, candidate creation, confidence, and sibling edge inference. |
src/brownfield/init.rs |
Adds run_init_from_code for creating the fixed brownfield init change. |
src/brownfield/refine.rs |
Adds timestamped refine change generation. |
src/brownfield/mod.rs |
Exposes new modules and writes generated proposal, delta, and contract artifacts. |
src/cli/mod.rs |
Wires init --from-code and refine into CLI dispatch. |
tests/phase_9_brownfield.rs |
Converts several brownfield planned tests into executable integration-style tests. |
Comments suppressed due to low confidence (4)
src/brownfield/mod.rs:86
- Edges are emitted inside each node block as
edge -> target, but the delta parser expects top-level edge operations in an edge section usingsource -> target "description". These sibling edges will either be ignored with the current missing section format or fail/misparse once the delta is put into a proper edge section.
for edge in &candidate.edges {
lines.push(format!(
" edge -> {} \"{}\"",
edge.target, edge.description
src/brownfield/init.rs:35
--forceremoves the existingbrownfield-initchange before discovery and the replacement write have succeeded. If discovery or any subsequent write fails, the user's previous change directory has already been deleted; write to a temporary location and swap it into place only after the new change is complete.
if change_dir.exists() && force {
std::fs::remove_dir_all(&change_dir).map_err(|e| CairnError::ChangeDiscovery {
path: change_dir.to_string_lossy().to_string(),
detail: e.to_string(),
})?;
src/brownfield/mod.rs:86
- Filesystem-derived strings are interpolated into quoted blueprint fields without escaping quotes or backslashes. A directory such as
src/user"apiwould generate an invalidblueprint.delta; use the same string escaping expected by the blueprint lexer before writing names, IDs, paths, and descriptions.
lines.push(format!(" path \"{}\"", candidate.path));
for edge in &candidate.edges {
lines.push(format!(
" edge -> {} \"{}\"",
edge.target, edge.description
src/brownfield/mod.rs:52
write_changecreates the destination withcreate_dir_all, which succeeds for an existing change directory and then overwrites files withstd::fs::write. Because this helper is public and used by collision guards, it should fail or atomically reserve the destination when it already exists unless the caller has explicitly removed it.
let change_dir = root.join("openspec/changes").join(change_id);
create_dir(&change_dir)?;
create_dir(&change_dir.join("contracts"))?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+74
to
+90
| let mut lines = vec!["# Blueprint delta\n".to_owned()]; | ||
| for candidate in &extraction.candidates { | ||
| lines.push(format!( | ||
| "+ {} \"{}\" id \"{}\" {{", | ||
| node_kind_from_path(&candidate.path), | ||
| candidate.name, | ||
| candidate.id | ||
| )); | ||
| lines.push(format!(" path \"{}\"", candidate.path)); | ||
| for edge in &candidate.edges { | ||
| lines.push(format!( | ||
| " edge -> {} \"{}\"", | ||
| edge.target, edge.description | ||
| )); | ||
| } | ||
| lines.push("}\n".to_owned()); | ||
| } |
Comment on lines
+22
to
+24
| let extraction = super::discovery::discover(root)?; | ||
| let change_id = unique_change_id(root, ×tamp()); | ||
| write_change(root, &change_id, &extraction)?; |
Comment on lines
+48
to
+52
| let secs = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_secs(); | ||
| format!("{secs}") |
Comment on lines
+98
to
+101
| let evidence: Vec<String> = files | ||
| .iter() | ||
| .map(|p| p.to_string_lossy().to_string()) | ||
| .collect(); |
Comment on lines
+93
to
+96
| let rel = dir.strip_prefix(root).unwrap_or(dir); | ||
| let rel_str = rel.to_string_lossy().to_string(); | ||
| let id = node_id_from_path(&rel_str); | ||
| let name = name_from_path(&rel_str); |
| candidate.name, | ||
| candidate.id | ||
| )); | ||
| lines.push(format!(" path \"{}\"", candidate.path)); |
Comment on lines
+24
to
+25
| let change_dir = root.join("openspec/changes").join(CHANGE_ID); | ||
| if change_dir.exists() && !force { |
Comment on lines
+172
to
+177
| fn is_ignored_dir(path: &Path) -> bool { | ||
| let name = path.file_name().map_or("", |n| n.to_str().unwrap_or("")); | ||
| matches!( | ||
| name, | ||
| "target" | "node_modules" | ".git" | ".cairn" | "openspec" | "meta" | "dist" | "build" | ||
| ) |
| use super::heuristics::path_derived_id; | ||
|
|
||
| /// Supported source file extensions for candidate discovery. | ||
| const SOURCE_EXTS: &[&str] = &["rs", "ts", "js", "py", "go"]; |
Comment on lines
+207
to
+222
| let n = candidates.len(); | ||
| for i in 0..n { | ||
| for j in (i + 1)..n { | ||
| if share_parent(&candidates[i].path, &candidates[j].path) { | ||
| let forward = DiscoveredEdge { | ||
| target: candidates[j].id.clone(), | ||
| description: "sibling module".to_owned(), | ||
| confidence: 1.0, | ||
| }; | ||
| let reverse = DiscoveredEdge { | ||
| target: candidates[i].id.clone(), | ||
| description: "sibling module".to_owned(), | ||
| confidence: 1.0, | ||
| }; | ||
| candidates[i].edges.push(forward); | ||
| candidates[j].edges.push(reverse); |
- Make evidence paths relative (consistent with candidate path) - Sort evidence for deterministic output across runs - Skip root-level directories that produce empty IDs - Remove invalid "Test" blueprint node kind, use "Module" instead - Register "refine" in EXTRA_CLI_COMMANDS and command_description - Normalize test fixture confidence to [0.0, 1.0] range - Use nanosecond timestamps in refine to prevent collision - Remove unnecessary 1s sleep in refine test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bstr)
- Skip symlinks during discovery to prevent cycle-induced stack overflow
- Bound unique_change_id counter to 999 iterations (prevent u32 overflow)
- Derive project root from --file flag instead of hardcoding Path::new(".")
- Match path segments (not substrings) in node_kind_from_path
- Correct misleading #[allow] comment on collect_source_files
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cli/mod.rs (1)
80-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse parsed project root for brownfield writes instead of hardcoded
".".Line 83 and Line 93 always target current working directory, so
--file <other-project>/cairn.blueprintcan write to the wrong repo.Proposed fix
let parsed = match parse_args(args) { Ok(parsed) => parsed, Err(result) => return result, }; + let root = parsed + .file + .parent() + .filter(|path| !path.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); if parsed.command == "init" { let from_code = parsed.command_args.iter().any(|a| a == "--from-code"); if from_code { let force = parsed.command_args.iter().any(|a| a == "--force"); - return match crate::brownfield::init::run_init_from_code(Path::new("."), force) { + return match crate::brownfield::init::run_init_from_code(root, force) { Ok(change_id) => ok(format!( "brownfield init complete; change written to openspec/changes/{change_id}/\n" )), Err(e) => err(1, &e.to_string()), }; } - return init_project(Path::new(".")); + return init_project(root); } if parsed.command == "refine" { - return match crate::brownfield::refine::run_refine(Path::new(".")) { + return match crate::brownfield::refine::run_refine(root) { Ok(change_id) => ok(format!( "refine complete; change written to openspec/changes/{change_id}/\n" )), Err(e) => err(1, &e.to_string()), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/mod.rs` around lines 80 - 94, The code uses a hardcoded Path::new(".") when calling brownfield functions (run_init_from_code and run_refine), causing writes to always go to the current working directory; instead pass the parsed project root from the parsed struct (e.g., parsed.project_root or parsed.project_root_path) into run_init_from_code and run_refine so the commands operate on the user-specified project root rather than "."; update the two call sites that reference Path::new(".") to use the parsed project root variable (and adjust types if necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/brownfield/mod.rs`:
- Around line 76-87: The DSL strings are built by injecting raw values
(candidate.name, candidate.path, edge.description) which can contain
quotes/newlines and break blueprint.delta; create and use a small helper like
escape_blueprint_string(s: &str) -> String that replaces backslash with "\\\\",
double-quote with "\\\"", newline with "\\n" (and optionally "\r" and "\t"),
then call that on candidate.name, candidate.path and edge.description when
building the format! lines (the lines that call
node_kind_from_path(&candidate.path) and push the path/name/edge entries) so all
inserted values are properly escaped.
---
Duplicate comments:
In `@src/cli/mod.rs`:
- Around line 80-94: The code uses a hardcoded Path::new(".") when calling
brownfield functions (run_init_from_code and run_refine), causing writes to
always go to the current working directory; instead pass the parsed project root
from the parsed struct (e.g., parsed.project_root or parsed.project_root_path)
into run_init_from_code and run_refine so the commands operate on the
user-specified project root rather than "."; update the two call sites that
reference Path::new(".") to use the parsed project root variable (and adjust
types if necessary).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9321014f-a51b-4568-b30b-97076196c0d9
📒 Files selected for processing (5)
src/brownfield/discovery.rssrc/brownfield/mod.rssrc/brownfield/refine.rssrc/cli/mod.rstests/phase_9_brownfield.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/brownfield/refine.rs
- src/brownfield/discovery.rs
Comment on lines
+76
to
+87
| lines.push(format!( | ||
| "+ {} \"{}\" id \"{}\" {{", | ||
| node_kind_from_path(&candidate.path), | ||
| candidate.name, | ||
| candidate.id | ||
| )); | ||
| lines.push(format!(" path \"{}\"", candidate.path)); | ||
| for edge in &candidate.edges { | ||
| lines.push(format!( | ||
| " edge -> {} \"{}\"", | ||
| edge.target, edge.description | ||
| )); |
There was a problem hiding this comment.
Escape blueprint string fields before writing delta entries.
candidate.name, candidate.path, and edge.description are injected raw into quoted DSL strings. A quote or newline in filesystem-derived values can produce invalid blueprint.delta.
Proposed fix
pub fn blueprint_delta(extraction: &Extraction) -> String {
let mut lines = vec!["# Blueprint delta\n".to_owned()];
for candidate in &extraction.candidates {
lines.push(format!(
"+ {} \"{}\" id \"{}\" {{",
node_kind_from_path(&candidate.path),
- candidate.name,
- candidate.id
+ escape_delta_string(&candidate.name),
+ escape_delta_string(&candidate.id)
));
- lines.push(format!(" path \"{}\"", candidate.path));
+ lines.push(format!(
+ " path \"{}\"",
+ escape_delta_string(&candidate.path)
+ ));
for edge in &candidate.edges {
lines.push(format!(
" edge -> {} \"{}\"",
- edge.target, edge.description
+ edge.target,
+ escape_delta_string(&edge.description)
));
}
lines.push("}\n".to_owned());
}
lines.join("\n")
}
+
+fn escape_delta_string(value: &str) -> String {
+ value
+ .replace('\\', r"\\")
+ .replace('"', r#"\""#)
+ .replace('\n', r"\n")
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/brownfield/mod.rs` around lines 76 - 87, The DSL strings are built by
injecting raw values (candidate.name, candidate.path, edge.description) which
can contain quotes/newlines and break blueprint.delta; create and use a small
helper like escape_blueprint_string(s: &str) -> String that replaces backslash
with "\\\\", double-quote with "\\\"", newline with "\\n" (and optionally "\r"
and "\t"), then call that on candidate.name, candidate.path and edge.description
when building the format! lines (the lines that call
node_kind_from_path(&candidate.path) and push the path/name/edge entries) so all
inserted values are properly escaped.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+36
to
+43
| let mut counter = 1u32; | ||
| loop { | ||
| let candidate = format!("{base}-{counter}"); | ||
| if !changes_dir.join(&candidate).exists() { | ||
| return candidate; | ||
| } | ||
| counter += 1; | ||
| } |
There was a problem hiding this comment.
Infinite loop without overflow protection. If all counter values are exhausted (extremely unlikely but possible), the loop will either panic on overflow (debug mode) or wrap around and loop indefinitely (release mode).
Add a safety limit:
let mut counter = 1u32;
loop {
let candidate = format!("{base}-{counter}");
if !changes_dir.join(&candidate).exists() {
return candidate;
}
counter = counter.checked_add(1).expect(
"Too many refine runs with the same timestamp; this should never happen"
);
}
Suggested change
| let mut counter = 1u32; | |
| loop { | |
| let candidate = format!("{base}-{counter}"); | |
| if !changes_dir.join(&candidate).exists() { | |
| return candidate; | |
| } | |
| counter += 1; | |
| } | |
| let mut counter = 1u32; | |
| loop { | |
| let candidate = format!("{base}-{counter}"); | |
| if !changes_dir.join(&candidate).exists() { | |
| return candidate; | |
| } | |
| counter = counter.checked_add(1).expect( | |
| "Too many refine runs with the same timestamp; this should never happen" | |
| ); | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
This was referenced Jul 25, 2026
This was referenced Aug 3, 2026
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
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.
Summary
Replaces PR #115 (rebased onto current dev with all merged PRs).
Ports brownfield extraction pipeline from experiment branch, with three review fixes:
infer_edgesnow adds edges in both directionsNew files:
src/brownfield/discovery.rs,init.rs,refine.rsExisting
heuristics.rsandonboard.rspreserved alongside.Bead:
cairn-1yk| GH #106Supersedes: #115
Test plan
cargo build(zero warnings)cargo clippy --all-targets --all-features -D warningscargo test(112+ tests pass)🤖 Generated with Claude Code