Skip to content

Refused a truncated source enumeration instead of rendering a short file - #27

Merged
dzykovic merged 2 commits into
mainfrom
fix/engine-enumeration-fails-closed
Sep 8, 2026
Merged

dzykovic merged 2 commits into
mainfrom
fix/engine-enumeration-fails-closed

Conversation

@dzykovic

@dzykovic dzykovic commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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:

engine/lib/common.sh: line 1357: printf: write error: Interrupted system call

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 a grep in CI, not anything the engine checks.

The defect is structural, not the EINTR. source_artifact_files streamed its result to stdout and every one of its eight call sites read it as done < <(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_files fills IS_SOURCE_FILES in the caller's shell:

  • no bash-level printf writes the list any more, so the observed failure path is gone;
  • the assembly loop runs in the current shell via a here-string, so a failure inside it is that shell's failure;
  • the one remaining pipeline (find | sort) goes through a command substitution, so pipefail plus the assignment's status make a dead find or sort an abort rather than a short answer;
  • an enumeration that cannot answer prints which directory it failed on and stops the run.

It also removes eight subshell forks per sync, which the engine's own rule cares about on Git Bash.

Type of change

  • CLI lifecycle or package behavior
  • Sync engine or adapter
  • Rule, agent, or skill content
  • Documentation
  • Refactor with no behavior change

Public CLI check

  • Uses only the established groups: init, sync, update, upgrade, package, adapter, status, registry — no command surface change
  • Keeps preview/apply behavior explicit for planned writes — unchanged
  • Covers automatic project alignment and CI refusal when project schema/content can change — unchanged
  • Keeps fresh-clone package restoration behind intelligence sync — unchanged

Verification

  • bash cli/tests/verify.sh reports verify ok and skipped nothing this change needed — lint and all ten suites green on Windows/Git Bash
  • intelligence status --check succeeds in the relevant fixture/project — this repository
  • A second intelligence sync produces no unexpected diff — this repository, worktree clean after the resync
  • Adapter changes were checked against the real tool format and preserve hand-authored sibling files — the agents adapter changed only in how it obtains its file list; its output is byte-identical (below)
  • User-facing docs and examples use the current public command groups

The refactor changes no output. Synced this repository before and after the change and compared: AGENTS.md byte-identical, the whole .claude/ tree byte-identical.

The regression test fails without the fix. e2e-negative section 17 puts a find stub on PATH that answers with a partial list and then exits nonzero for one directory — the shape an interrupted write produces. Reproduced by hand on both sides:

exit status line AGENTS.md skills listed
before 0 IS_STATUS=ok 514 bytes, silently shortened 1 of 2
after 1 ERROR: cannot enumerate skills under '…' 573 bytes, restored intact 2 of 2

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

  • The PR records its change under one concrete CHANGELOG.md version; there is no [Unreleased] section or duplicated release date — added to the pending ## [0.14.0] under ### Fixed
  • The current engine/VERSION release state was checked: append while pending, or select the next SemVer after publication — v0.14.0 has no GitHub Release, so this appends to it rather than selecting a new version
  • If a new version was selected, every example's schema_version and exact @ainova-systems/sync pin changed with it — not applicable, no version selected
  • No stable release is implied or published without owner approval

Notes for reviewers

  • Why not retry the interrupted write. Retrying makes this one producer survive EINTR and leaves the class intact: any other producer feeding a renderer through a process substitution still fails the same silent way. The rule is now the fix — a list that decides what gets rendered is assembled where its failures are the shell's.
  • Bash baseline. No mapfile, no namerefs; macOS ships Bash 3.2. The empty-array reads use ${arr[@]+"${arr[@]}"} so set -u is satisfied there too.
  • What is still streamed. Other process substitutions in the engine feed per-file passes whose input is already a known array, not the decision of what to render. They are not in this class, and none of them was changed.
  • Rule updated with the code. intelligence/rules/engine.md now states the constraint and names the failure mode, so the next enumeration is written the same way.
  • Rollback. One commit, no data or schema change, no output change; reverting restores the previous behavior exactly.

Copilot AI lite review requested due to automatic review settings September 8, 2026 11:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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_files usage with read_source_artifact_files that fills IS_SOURCE_FILES in 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.

Comment thread engine/adapters/agents.sh
Comment thread engine/lib/common.sh Outdated
@dzykovic dzykovic added the ai:ready-to-merge CI green, reviews resolved, awaiting owner approval label Sep 8, 2026
@dzykovic
dzykovic merged commit a9cf448 into main Sep 8, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai:ready-to-merge CI green, reviews resolved, awaiting owner approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants