Refused a truncated source enumeration instead of rendering a short file - #27
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Several new call sites expand IS_SOURCE_FILES unquoted, which can word-split/glob-expand paths and corrupt the enumerated file list.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the sync engine’s source-file enumeration so adapters fail closed instead of silently rendering incomplete outputs when enumeration is truncated (e.g., via interrupted writes / lost producer exit status), and adds a regression test plus documentation/changelog updates to lock the behavior in.
Changes:
- Replaced streaming
source_artifact_filesusage withread_source_artifact_filesthat fillsIS_SOURCE_FILESin the caller’s shell and aborts on enumeration failures. - Updated the agents adapter and shared engine helpers to consume the in-shell array rather than process substitutions.
- Added an end-to-end negative test reproducing a “partial list then failure” enumeration shape, and recorded the fix in docs/changelog.
File summaries
| File | Description |
|---|---|
| intelligence/rules/engine.md | Documents the “fail closed” rule and why process-substitution streaming can silently truncate enumerations. |
| engine/lib/common.sh | Implements read_source_artifact_files and updates call sites to consume the in-shell IS_SOURCE_FILES array. |
| engine/adapters/agents.sh | Switches agents/rules/skills enumeration to read_source_artifact_files. |
| cli/tests/e2e-negative.sh | Adds regression coverage to ensure truncated/failed enumeration aborts and leaves prior outputs intact. |
| CHANGELOG.md | Records the fix under the pending version’s “Fixed” section. |
Review details
Suppressed comments (4)
engine/lib/common.sh:1426
- This array assignment uses an unquoted array expansion, which can word-split and glob-expand file paths. Use a quoted array expansion so each path remains a single element.
read_source_artifact_files "$repo_root" "$config_file" "rules"
rule_files=(${IS_SOURCE_FILES[@]+"${IS_SOURCE_FILES[@]}"})
if [ "${#rule_files[@]}" -gt 0 ]; then
engine/lib/common.sh:1441
- agent_files and skill_files are populated via an unquoted array expansion, which can word-split and glob-expand paths. Quote the expansion to preserve exact file paths.
read_source_artifact_files "$repo_root" "$config_file" "agents"
agent_files=(${IS_SOURCE_FILES[@]+"${IS_SOURCE_FILES[@]}"})
read_source_artifact_files "$repo_root" "$config_file" "skills"
skill_files=(${IS_SOURCE_FILES[@]+"${IS_SOURCE_FILES[@]}"})
engine/adapters/agents.sh:106
- skill_files is built with an unquoted array expansion, which can re-split or glob-expand paths. Quote the expansion so SKILL.md paths are passed to frontmatter_index correctly.
read_source_artifact_files "$repo_root" "$config_file" "skills"
skill_files=(${IS_SOURCE_FILES[@]+"${IS_SOURCE_FILES[@]}"})
engine/adapters/agents.sh:148
- Unquoted array expansion here can word-split and glob-expand rule file paths, potentially corrupting the rendered list and the global_rule_files bookkeeping. Quote the expansion to preserve each path.
local -a files=()
read_source_artifact_files "$repo_root" "$config_file" "rules"
files=(${IS_SOURCE_FILES[@]+"${IS_SOURCE_FILES[@]}"})
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
A smoke job on the macOS runner failed while PR #26 was in review, and the cause was not that PR. The log carried one line from the engine:
The write that got interrupted was building the list of files to render. The list came up short, the AGENTS.md adapter rendered 3 skills where the antigravity adapter in the same run rendered 13, the file came out at 1854 bytes, and the run reported
IS_STATUS=ok. The assertion that caught it was agrepin CI, not anything the engine checks.The defect is structural, not the EINTR.
source_artifact_filesstreamed its result to stdout and every one of its eight call sites read it asdone < <(source_artifact_files …). A process substitution discards its writer's exit status, so a producer that dies mid-list is indistinguishable from a producer that finished — a truncated list arrives as "these are the files". Since that list decides what every adapter renders, the only visible symptom is a smaller output, which is exactly what a smaller project looks like.The fix removes the streaming.
read_source_artifact_filesfillsIS_SOURCE_FILESin the caller's shell:printfwrites the list any more, so the observed failure path is gone;find | sort) goes through a command substitution, sopipefailplus the assignment's status make a deadfindorsortan abort rather than a short answer;It also removes eight subshell forks per sync, which the engine's own rule cares about on Git Bash.
Type of change
Public CLI check
init,sync,update,upgrade,package,adapter,status,registry— no command surface changeintelligence sync— unchangedVerification
bash cli/tests/verify.shreportsverify okand skipped nothing this change needed —lintand all ten suites green on Windows/Git Bashintelligence status --checksucceeds in the relevant fixture/project — this repositoryintelligence syncproduces no unexpected diff — this repository, worktree clean after the resyncThe refactor changes no output. Synced this repository before and after the change and compared:
AGENTS.mdbyte-identical, the whole.claude/tree byte-identical.The regression test fails without the fix.
e2e-negativesection 17 puts afindstub onPATHthat answers with a partial list and then exits nonzero for one directory — the shape an interrupted write produces. Reproduced by hand on both sides:IS_STATUS=okERROR: cannot enumerate skills under '…'The test asserts all four: nonzero exit, the named directory, no
IS_STATUS=ok, and that the transaction left the previous complete file in place.Versioned artifacts
CHANGELOG.mdversion; there is no[Unreleased]section or duplicated release date — added to the pending## [0.14.0]under### Fixedengine/VERSIONrelease state was checked: append while pending, or select the next SemVer after publication —v0.14.0has no GitHub Release, so this appends to it rather than selecting a new versionschema_versionand exact@ainova-systems/syncpin changed with it — not applicable, no version selectedNotes for reviewers
mapfile, no namerefs; macOS ships Bash 3.2. The empty-array reads use${arr[@]+"${arr[@]}"}soset -uis satisfied there too.intelligence/rules/engine.mdnow states the constraint and names the failure mode, so the next enumeration is written the same way.