Skip to content

Compile: write into directory when --output is a directory#331

Merged
jamesadevine merged 4 commits intomainfrom
copilot/place-compiled-agent-file
Apr 28, 2026
Merged

Compile: write into directory when --output is a directory#331
jamesadevine merged 4 commits intomainfrom
copilot/place-compiled-agent-file

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 28, 2026

Previously ado-aw compile foo.md --output some-dir/ treated some-dir/ as the literal output filename, producing a file named some-dir regardless of whether a directory of that name existed. Callers wanting "default name in this directory" had to repeat the stem.

Changes

  • src/compile/mod.rs: In compile_pipeline, when output_path resolves to an existing directory, join it with the default filename derived from the input markdown's stem (foo.md<dir>/foo.lock.yml). Non-directory paths are written verbatim as before.
  • Test: Added a tokio::test covering the directory case end-to-end, asserting both that the file lands at the expected path and that it contains the @ado-aw source header marker.
  • Docs: Updated the --output doc comment in src/main.rs and the compile section in AGENTS.md.

Example

# Before: produced a file literally named "out"
ado-aw compile path/to/foo.md --output out/

# After: produces out/foo.lock.yml
ado-aw compile path/to/foo.md --output out/

# Unchanged: explicit file path still respected
ado-aw compile path/to/foo.md --output out/custom.yml

@jamesadevine
Copy link
Copy Markdown
Collaborator

/rust-review

1 similar comment
@jamesadevine
Copy link
Copy Markdown
Collaborator

/rust-review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 28, 2026

Rust PR Reviewer completed successfully!

@github-actions
Copy link
Copy Markdown
Contributor

🔍 Rust PR Review

Summary: Looks good — clean, correct change with appropriate tests and docs. A couple of minor observations worth noting.

Findings

⚠️ Suggestions

  • src/compile/mod.rs:89-93default_filename is computed eagerly and unconditionally, but it's only needed when output_path is Some(p) and p.is_dir(). When output_path is None or the given path is a file, the computation is wasted. More importantly, the error path ("Invalid input path") fires even in the None branch where the path couldn't have been user-supplied. Consider computing it lazily inside the is_dir() arm, or at least moving it inside the Some(p) arm:

    let yaml_output_path = match output_path {
        Some(p) => {
            let p = PathBuf::from(p);
            if p.is_dir() {
                let stem = input_path
                    .with_extension("yml")
                    .file_name()
                    .map(PathBuf::from)
                    .with_context(|| format!("Invalid input path: {}", input_path.display()))?;
                p.join(stem)
            } else {
                p
            }
        }
        None => input_path.with_extension("yml"),
    };
  • src/compile/mod.rs:699-705 (test) — SystemTime::now().duration_since(UNIX_EPOCH).unwrap() will panic if the system clock is before 1970. In test code .unwrap() is fine, but std::process::id() alone is usually sufficient for uniqueness in the same test run. Alternatively, the tempfile crate (if already in Cargo.toml) would give automatic cleanup and proper tmp-dir hygiene.

  • src/compile/mod.rs (test) — The test only asserts the output file exists, not that it contains valid YAML or the expected header marker. That said, the full compile path is exercised and a compile error would already be caught by .expect("compile_pipeline should succeed"), so this is acceptable for an integration-style test. A follow-up could assert on a minimal content invariant.

✅ What Looks Good

  • The method chain input_path.with_extension("yml").file_name().map(PathBuf::from) is idiomatic — the intermediate PathBuf temporary lives for the full statement so there's no dangling-reference issue.
  • p.is_dir() correctly handles the case where the caller passes a directory with a trailing slash (OS normalises it).
  • Docs updated in both AGENTS.md and the clap #[arg] doc-comment consistently.
  • No unwrap()/expect() on user-facing paths; the fallible file_name() result is properly propagated via with_context.
  • Test exercises the full async compile_pipeline path end-to-end, which catches integration issues that unit tests would miss.

Generated by Rust PR Reviewer for issue #331 · ● 393.5K ·

@jamesadevine
Copy link
Copy Markdown
Collaborator

@copilot Address feedback and suggestions

…nt in test

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/f02521ba-67d6-456d-af2a-a228d50389ab

Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 28, 2026

@copilot Address feedback and suggestions

Addressed in 62ab8f7: moved the default_filename computation inside the is_dir() arm so it's only done (and can only error) when needed, and strengthened the test to also assert the compiled YAML contains the @ado-aw header marker.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh gh pr list --head copilot/place-compiled-agent-file --json number,title (http block)

If you need me to access, download, or install something from one of these locations, you can either:

@jamesadevine
Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

Copilot AI and others added 2 commits April 28, 2026 12:35
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 28, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in 2333669: merged main and combined the new .lock.yml default extension with the directory-output handling. Directory-output now produces e.g. <dir>/foo.lock.yml. Doc strings in AGENTS.md and src/main.rs updated to match.

@jamesadevine jamesadevine merged commit 14e7b7f into main Apr 28, 2026
13 checks passed
@jamesadevine jamesadevine deleted the copilot/place-compiled-agent-file branch April 28, 2026 13:09
jamesadevine added a commit that referenced this pull request Apr 28, 2026
This is an empty commit to trigger release-please to open a release PR
so that the changelog can be manually updated to include changes from
commits #330, #331, and #332 which were merged without Conventional
Commit prefixes and were silently dropped from the changelog.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants