Conversation
* Fix: include sub-app Towerfiles in packages Previously, should_ignore excluded every file named 'Towerfile' from the package. This broke apps that contain sub-apps with their own Towerfiles, since those files were silently dropped. The fix introduces FileResolver, a struct that owns the base_dir and the root Towerfile path, and uses it to make all resolution decisions. Only the exact Towerfile used to build the package is excluded; any Towerfile living in a sub-directory is included as normal app content. Changes: - FileResolver struct owns base_dir and root_towerfile - Resolution logic lives on FileResolver as async methods (resolve_glob, resolve_path) rather than standalone free functions - should_ignore compares by path identity, not by file name - Package::build canonicalizes spec.towerfile_path and delegates to FileResolver for both the glob loop and the import-path loop - New integration test it_includes_subapp_towerfiles_but_excludes_root_towerfile verifies the exact scenario described above * chore: Rollback change to tests, and fix bug that was introduced
bradhe
requested review from
Copilot,
giray123,
jo-sm,
konstantinoscs,
sammuti and
socksy
📝 WalkthroughWalkthroughAdds a 30s RUN_START_TIMEOUT and new Error::RunStartTimeout to bound run startup waiting; updates follow_logs and wait_for_run_start to return on timeout. Introduces FileResolver in tower-package to centralize path whitelisting, logical path mapping, and glob/path resolution; adds related packaging tests and minor error messaging. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Packager
participant Resolver as FileResolver
participant FS as Filesystem
participant Manifest as Manifest/Archive
CLI->>Resolver: provide base_dir + canonical import paths
Resolver->>FS: walk/glob resolve paths (apply should_ignore)
FS-->>Resolver: return physical file list
Resolver->>Resolver: compute logical_path mapping (module alignment)
Resolver-->>CLI: return mapping physical -> logical
CLI->>Manifest: add files under modules/... and update import_paths
Manifest-->>CLI: package assembled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Release v0.3.53 focuses on improving CLI robustness when following logs and adjusting packaging behavior to include sub-app Towerfiles while still excluding the root Towerfile.
Changes:
- Add a run-start timeout when following/streaming logs to avoid hanging indefinitely.
- Update tower-package file resolution to exclude only the root Towerfile and to canonicalize/resolve import paths via a new resolver.
- Add a regression test ensuring sub-app Towerfiles are included in the packaged app.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tower-package/tests/package_test.rs | Adds coverage for including sub-app Towerfiles while excluding the root Towerfile under app/. |
| crates/tower-package/src/lib.rs | Refactors path/glob resolution into FileResolver, changes Towerfile ignore behavior, and adjusts import-path handling. |
| crates/tower-cmd/src/run.rs | Adds a timeout to wait_for_run_start and surfaces a dedicated timeout error. |
| crates/tower-cmd/src/error.rs | Introduces RunStartTimeout error variant/message. |
| crates/tower-cmd/src/apps.rs | Adds a timeout while waiting for a run to start in apps logs --follow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/tower-package/src/lib.rs
Outdated
Comment on lines
+544
to
+545
| for entry in glob(&path_str).unwrap() { | ||
| self.resolve_path(&entry.unwrap(), file_paths).await; |
There was a problem hiding this comment.
glob(&path_str).unwrap() and entry.unwrap() will panic on invalid glob patterns or glob iteration errors (which can come from user-provided file_globs). Prefer returning a structured error instead of panicking so packaging fails gracefully with a useful message.
Suggested change
| for entry in glob(&path_str).unwrap() { | |
| self.resolve_path(&entry.unwrap(), file_paths).await; | |
| let paths = match glob(&path_str) { | |
| Ok(paths) => paths, | |
| Err(e) => { | |
| debug!("failed to parse glob pattern {}: {}", path_str, e); | |
| return; | |
| } | |
| }; | |
| for entry in paths { | |
| match entry { | |
| Ok(p) => { | |
| self.resolve_path(&p, file_paths).await; | |
| } | |
| Err(e) => { | |
| debug!("error while iterating glob results for {}: {}", path_str, e); | |
| continue; | |
| } | |
| } |
Comment on lines
+310
to
+311
| output::error("Timed out waiting for run to start. The runner may be unavailable."); | ||
| return; |
There was a problem hiding this comment.
On timeout this path logs an error and returns, but do_logs will then exit successfully (status 0). If a timeout should be treated as command failure, consider using output::die(...) or propagating an error so the CLI exits non-zero and callers can detect the failure.
Suggested change
| output::error("Timed out waiting for run to start. The runner may be unavailable."); | |
| return; | |
| output::die("Timed out waiting for run to start. The runner may be unavailable."); |
Comment on lines
+521
to
+535
| fn logical_path<'a>(&self, physical_path: &'a Path) -> Option<&'a Path> { | ||
| if let Ok(p) = physical_path.strip_prefix(&self.base_dir) { | ||
| return Some(p); | ||
| } | ||
|
|
||
| // Try each import path's parent as a prefix. This allows files within import paths | ||
| // (which may live outside base_dir) to be resolved with logical paths that preserve | ||
| // the import directory name (e.g. "shared_lib/foo.py"). | ||
| for import_path in &self.import_paths { | ||
| if let Some(parent) = import_path.parent() { | ||
| if let Ok(p) = physical_path.strip_prefix(parent) { | ||
| return Some(p); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
FileResolver::logical_path strips base_dir first, so if an import_path is inside (but not directly under) base_dir (e.g. libs/shared), module files will resolve to libs/shared/... and get packaged under modules/libs/shared/..., while the manifest entry is still modules/shared. This breaks PYTHONPATH resolution at runtime. Consider resolving module import files relative to each import_path.parent() (as before) even when the import path lives within base_dir (e.g., pass an explicit logical root into resolve_path, or have a separate resolver method for import paths).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tower-cmd/src/apps.rs (1)
288-321:⚠️ Potential issue | 🟠 MajorThe new startup timeout does not cover the
describe_runcalls.Line 289 and Line 319 still await
api::describe_runoutside the 30-second cap, so a slow or stalled poll can keepapps logs --followblocked well past the advertised timeout. Therunflow already bounds the entire startup wait incrates/tower-cmd/src/run.rs:747-761; this path needs the same shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/apps.rs` around lines 288 - 321, The describe_run calls in this loop (the initial one that sets run and the inner-loop call inside the "not started" poll) are not bounded by the RUN_START_TIMEOUT and can block past the advertised startup cap; change both api::describe_run awaits to use a timeout pattern like in run.rs so each describe_run is wrapped with tokio::time::timeout using the remaining time (RUN_START_TIMEOUT - wait_started.elapsed() for the inner loop, or a full RUN_START_TIMEOUT for the initial check), and on timeout treat it the same as a start-timeout error path (call output::error("Timed out waiting for run to start...") and return or use output::tower_error_and_die consistently). Ensure you reference the existing wait_started, RUN_START_TIMEOUT, and should_notify_run_wait logic so notification and timeout semantics remain unchanged.
🧹 Nitpick comments (2)
crates/tower-package/src/lib.rs (2)
569-574: Unwrappingread_dirandnext_entrycan panic on I/O errors.These operations can fail due to permission issues, concurrent file system modifications, or other I/O errors. Panicking here would crash the package build process unexpectedly.
♻️ Proposed fix with graceful error handling
if physical_path.is_dir() { - let mut entries = tokio::fs::read_dir(&physical_path).await.unwrap(); - - while let Some(entry) = entries.next_entry().await.unwrap() { - queue.push_back(entry.path()); + match tokio::fs::read_dir(&physical_path).await { + Ok(mut entries) => { + while let Ok(Some(entry)) = entries.next_entry().await { + queue.push_back(entry.path()); + } + } + Err(e) => { + debug!( + " - skipping directory {}: {}", + physical_path.display(), + e + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-package/src/lib.rs` around lines 569 - 574, The code currently unwraps tokio::fs::read_dir(&physical_path).await and entries.next_entry().await which can panic on I/O errors; change this to handle errors gracefully by matching the Result from read_dir and from entries.next_entry(): on read_dir error, log or skip this directory (do not panic) and continue the loop; on next_entry error, log and break or continue as appropriate instead of unwrap; keep using queue.push_back(entry.path()) only when entry is Ok. Update the block around physical_path, entries, read_dir, next_entry, and queue.push_back to propagate or handle errors safely (using if let Ok(...) / match) so the package build doesn't crash on filesystem errors.
544-546: Unwrapping glob errors can panic on invalid patterns or I/O failures.Both
glob().unwrap()andentry.unwrap()will panic if the glob pattern is malformed or if there's an I/O error during directory enumeration. Consider propagating errors or logging and continuing gracefully.♻️ Proposed fix with error handling
async fn resolve_glob(&self, path: PathBuf, file_paths: &mut HashMap<PathBuf, PathBuf>) { let path_str = extract_glob_path(path); debug!("resolving glob pattern: {}", path_str); - for entry in glob(&path_str).unwrap() { - self.resolve_path(&entry.unwrap(), file_paths).await; + let entries = match glob(&path_str) { + Ok(entries) => entries, + Err(e) => { + debug!("invalid glob pattern {}: {}", path_str, e); + return; + } + }; + + for entry in entries { + match entry { + Ok(path) => self.resolve_path(&path, file_paths).await, + Err(e) => debug!("glob entry error: {}", e), + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-package/src/lib.rs` around lines 544 - 546, The loop currently calls glob(&path_str).unwrap() and entry.unwrap(), which can panic; change it to handle errors from glob() and from each entry result instead: call glob(&path_str) and match or use if let Ok(paths) = ... to log or return the PatternError/GlobError, then iterate over paths and for each entry handle Err by logging and continuing (or propagating) and pass the successful Path to self.resolve_path(&entry, file_paths).await; reference the glob(&path_str) call and the use of self.resolve_path and ensure no unwrap() remains so malformed patterns or I/O errors are handled gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tower-package/src/lib.rs`:
- Around line 485-489: The should_ignore(&self, p: &PathBuf) check incorrectly
uses Path::ends_with(".pyc") which compares path components rather than the file
suffix; change the logic in should_ignore to test the file extension instead
(e.g., use p.extension() and compare to "pyc" or map to_str()/to_string_lossy()
and check suffix) so compiled Python files are correctly ignored; update the
branch that currently checks p.ends_with(".pyc") to use p.extension().map(|ext|
ext == "pyc").unwrap_or(false) (or equivalent) within the should_ignore
function.
---
Outside diff comments:
In `@crates/tower-cmd/src/apps.rs`:
- Around line 288-321: The describe_run calls in this loop (the initial one that
sets run and the inner-loop call inside the "not started" poll) are not bounded
by the RUN_START_TIMEOUT and can block past the advertised startup cap; change
both api::describe_run awaits to use a timeout pattern like in run.rs so each
describe_run is wrapped with tokio::time::timeout using the remaining time
(RUN_START_TIMEOUT - wait_started.elapsed() for the inner loop, or a full
RUN_START_TIMEOUT for the initial check), and on timeout treat it the same as a
start-timeout error path (call output::error("Timed out waiting for run to
start...") and return or use output::tower_error_and_die consistently). Ensure
you reference the existing wait_started, RUN_START_TIMEOUT, and
should_notify_run_wait logic so notification and timeout semantics remain
unchanged.
---
Nitpick comments:
In `@crates/tower-package/src/lib.rs`:
- Around line 569-574: The code currently unwraps
tokio::fs::read_dir(&physical_path).await and entries.next_entry().await which
can panic on I/O errors; change this to handle errors gracefully by matching the
Result from read_dir and from entries.next_entry(): on read_dir error, log or
skip this directory (do not panic) and continue the loop; on next_entry error,
log and break or continue as appropriate instead of unwrap; keep using
queue.push_back(entry.path()) only when entry is Ok. Update the block around
physical_path, entries, read_dir, next_entry, and queue.push_back to propagate
or handle errors safely (using if let Ok(...) / match) so the package build
doesn't crash on filesystem errors.
- Around line 544-546: The loop currently calls glob(&path_str).unwrap() and
entry.unwrap(), which can panic; change it to handle errors from glob() and from
each entry result instead: call glob(&path_str) and match or use if let
Ok(paths) = ... to log or return the PatternError/GlobError, then iterate over
paths and for each entry handle Err by logging and continuing (or propagating)
and pass the successful Path to self.resolve_path(&entry, file_paths).await;
reference the glob(&path_str) call and the use of self.resolve_path and ensure
no unwrap() remains so malformed patterns or I/O errors are handled gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bf3e12b-584c-4948-a0c0-6b947d572494
📒 Files selected for processing (5)
crates/tower-cmd/src/apps.rscrates/tower-cmd/src/error.rscrates/tower-cmd/src/run.rscrates/tower-package/src/lib.rscrates/tower-package/tests/package_test.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tower-package/src/lib.rs`:
- Around line 536-543: The loop over self.import_paths currently accepts any
file under import_path.parent(), allowing files outside the declared import
root; change the check to verify physical_path is inside the specific
import_path before stripping the parent. Concretely, inside the loop over
import_path in &self.import_paths only proceed if
physical_path.strip_prefix(import_path) (or an equivalent starts-with/membership
test against import_path) succeeds, and then compute the returned relative path
by stripping import_path.parent() (using import_path.parent() and
physical_path.strip_prefix(parent) as before) so only files actually under the
configured import_path are allowed; update the logic around
import_path.parent(), physical_path.strip_prefix(...), and the return Some(p)
accordingly.
- Around line 574-599: The resolve_path function currently canonicalizes and
recursively traverses directories without tracking visited canonical paths and
uses unwrap() on fallible operations; change it to be fallible (return a Result)
and replace unwraps with ? or proper error handling, add a visited
HashSet<PathBuf> (canonical paths) to skip already-seen directories before
pushing onto queue, check canonical_path and physical_path via match/if let to
handle errors rather than unwrap(), and ensure read_dir/next_entry errors are
propagated; update references to file_paths and queue so you record mappings
only for successfully canonicalized entries and avoid enqueueing duplicates by
checking the visited set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fb8ae6a-7ea3-45e0-8e40-4ec6b3323a11
📒 Files selected for processing (4)
crates/tower-cmd/src/output.rscrates/tower-package/src/error.rscrates/tower-package/src/lib.rscrates/tower-package/tests/package_test.rs
Comment on lines
+536
to
+543
| // Try each import path's parent as a prefix. This allows files within import paths | ||
| // (which may live outside base_dir) to be resolved with logical paths that preserve | ||
| // the import directory name (e.g. "shared_lib/foo.py"). | ||
| for import_path in &self.import_paths { | ||
| if let Some(parent) = import_path.parent() { | ||
| if let Ok(p) = physical_path.strip_prefix(parent) { | ||
| return Some(p); | ||
| } |
There was a problem hiding this comment.
Restrict the allowlist to the configured import root.
This currently accepts any file under import_path.parent(), not just files inside the import path. With import_paths = ["../shared"], a source glob like ../secret.env would be treated as valid package content and land under app/. Check membership against import_path before stripping the parent prefix.
🔒 Suggested fix
for import_path in &self.import_paths {
- if let Some(parent) = import_path.parent() {
- if let Ok(p) = physical_path.strip_prefix(parent) {
- return Some(p);
+ if physical_path.starts_with(import_path) {
+ if let Some(parent) = import_path.parent() {
+ if let Ok(p) = physical_path.strip_prefix(parent) {
+ return Some(p);
+ }
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Try each import path's parent as a prefix. This allows files within import paths | |
| // (which may live outside base_dir) to be resolved with logical paths that preserve | |
| // the import directory name (e.g. "shared_lib/foo.py"). | |
| for import_path in &self.import_paths { | |
| if let Some(parent) = import_path.parent() { | |
| if let Ok(p) = physical_path.strip_prefix(parent) { | |
| return Some(p); | |
| } | |
| // Try each import path's parent as a prefix. This allows files within import paths | |
| // (which may live outside base_dir) to be resolved with logical paths that preserve | |
| // the import directory name (e.g. "shared_lib/foo.py"). | |
| for import_path in &self.import_paths { | |
| if physical_path.starts_with(import_path) { | |
| if let Some(parent) = import_path.parent() { | |
| if let Ok(p) = physical_path.strip_prefix(parent) { | |
| return Some(p); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tower-package/src/lib.rs` around lines 536 - 543, The loop over
self.import_paths currently accepts any file under import_path.parent(),
allowing files outside the declared import root; change the check to verify
physical_path is inside the specific import_path before stripping the parent.
Concretely, inside the loop over import_path in &self.import_paths only proceed
if physical_path.strip_prefix(import_path) (or an equivalent
starts-with/membership test against import_path) succeeds, and then compute the
returned relative path by stripping import_path.parent() (using
import_path.parent() and physical_path.strip_prefix(parent) as before) so only
files actually under the configured import_path are allowed; update the logic
around import_path.parent(), physical_path.strip_prefix(...), and the return
Some(p) accordingly.
Comment on lines
+574
to
+599
| async fn resolve_path(&self, path: &PathBuf, file_paths: &mut HashMap<PathBuf, PathBuf>) { | ||
| let mut queue = VecDeque::new(); | ||
| queue.push_back(path.to_path_buf()); | ||
|
|
||
| while let Some(current_path) = queue.pop_front() { | ||
| let canonical_path = current_path.canonicalize(); | ||
|
|
||
| if canonical_path.is_err() { | ||
| debug!( | ||
| " - skipping path {}: {}", | ||
| current_path.display(), | ||
| canonical_path.unwrap_err() | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| // We can safely unwrap this because we understand that it's not going to fail at this | ||
| // point. | ||
| let physical_path = canonical_path.unwrap(); | ||
|
|
||
| if physical_path.is_dir() { | ||
| let mut entries = tokio::fs::read_dir(&physical_path).await.unwrap(); | ||
|
|
||
| while let Some(entry) = entries.next_entry().await.unwrap() { | ||
| queue.push_back(entry.path()); | ||
| } |
There was a problem hiding this comment.
Make directory traversal cycle-safe and fallible.
A symlinked directory back into an already-visited subtree will be canonicalized and traversed forever here because nothing records visited canonical paths. The unwrap() calls also turn normal read_dir failures into panics instead of package errors.
♻️ Suggested direction
-use std::collections::{HashMap, VecDeque};
+use std::collections::{HashMap, HashSet, VecDeque};
...
- async fn resolve_path(&self, path: &PathBuf, file_paths: &mut HashMap<PathBuf, PathBuf>) {
+ async fn resolve_path(
+ &self,
+ path: &PathBuf,
+ file_paths: &mut HashMap<PathBuf, PathBuf>,
+ ) -> Result<(), Error> {
let mut queue = VecDeque::new();
+ let mut visited = HashSet::new();
queue.push_back(path.to_path_buf());
while let Some(current_path) = queue.pop_front() {
- let canonical_path = current_path.canonicalize();
-
- if canonical_path.is_err() {
+ let physical_path = match current_path.canonicalize() {
+ Ok(path) => path,
+ Err(err) => {
+ debug!(" - skipping path {}: {}", current_path.display(), err);
+ continue;
+ }
+ };
+
+ if !visited.insert(physical_path.clone()) {
+ continue;
+ }
-
- // We can safely unwrap this because we understand that it's not going to fail at this
- // point.
- let physical_path = canonical_path.unwrap();
if physical_path.is_dir() {
- let mut entries = tokio::fs::read_dir(&physical_path).await.unwrap();
+ let mut entries = tokio::fs::read_dir(&physical_path).await?;
- while let Some(entry) = entries.next_entry().await.unwrap() {
+ while let Some(entry) = entries.next_entry().await? {
queue.push_back(entry.path());
}
} else {
...
}
}
+
+ Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tower-package/src/lib.rs` around lines 574 - 599, The resolve_path
function currently canonicalizes and recursively traverses directories without
tracking visited canonical paths and uses unwrap() on fallible operations;
change it to be fallible (return a Result) and replace unwraps with ? or proper
error handling, add a visited HashSet<PathBuf> (canonical paths) to skip
already-seen directories before pushing onto queue, check canonical_path and
physical_path via match/if let to handle errors rather than unwrap(), and ensure
read_dir/next_entry errors are propagated; update references to file_paths and
queue so you record mappings only for successfully canonicalized entries and
avoid enqueueing duplicates by checking the visited set.
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 by CodeRabbit
Bug Fixes
Improvements
Tests
Chores