Skip to content

Conversation

@asmacdo
Copy link
Member

@asmacdo asmacdo commented Jan 31, 2026

Internal changes:

  • Move DUCT_OUTPUT_PREFIX and EXECUTION_SUMMARY_FORMAT from duct_main.py to cli.py, renamed to DEFAULT_OUTPUT_PREFIX and DEFAULT_SUMMARY_FORMAT
  • Break circular import by adding --output-prefix / -p to con-duct ls, so ls.py receives the prefix via args instead of importing it from duct_main

Fix latent bug:

  • the output prefix default was evaluated at import time (before .env files were loaded); now os.getenv() runs at parser creation time inside main()

Closes #386

🤖 Generated with Claude Code

Move DUCT_OUTPUT_PREFIX and EXECUTION_SUMMARY_FORMAT out of duct_main.py
and into cli.py as DEFAULT_OUTPUT_PREFIX and DEFAULT_SUMMARY_FORMAT.
These are CLI defaults, not execution logic.

Break the circular import (cli -> ls -> duct_main -> cli) by adding
--output-prefix / -p to con-duct ls, passing the value through args
so ls.py no longer imports from duct_main.

Also fix a latent bug: DEFAULT_OUTPUT_PREFIX is now a plain string
constant, with os.getenv() evaluated at parser creation time (after
.env files are loaded) rather than at import time.

Closes con#386

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 31, 2026 16:23
@codecov
Copy link

codecov bot commented Jan 31, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.47%. Comparing base (3756590) to head (30d03b9).

Files with missing lines Patch % Lines
src/con_duct/ls.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
- Coverage   91.81%   91.47%   -0.34%     
==========================================
  Files          15       15              
  Lines        1112     1115       +3     
  Branches      138      138              
==========================================
- Hits         1021     1020       -1     
- Misses         69       72       +3     
- Partials       22       23       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds an --output-prefix (-p) option to con-duct ls and relocates CLI default values into cli.py to remove a circular import and ensure defaults are read after .env files are loaded.

Changes:

  • Move CLI default constants from duct_main.py to cli.py (DEFAULT_OUTPUT_PREFIX, DEFAULT_SUMMARY_FORMAT).
  • Add --output-prefix/-p to con-duct ls and refactor ls.py to derive the glob search prefix from args instead of importing from duct_main.
  • Update tests to use the new constants and cover custom ls --output-prefix behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/con_duct/cli.py Introduces new default constants and uses env-evaluated defaults at parser creation; adds ls --output-prefix.
src/con_duct/duct_main.py Removes CLI-default constants from the execution module.
src/con_duct/ls.py Stops importing defaults and uses args.output_prefix to build the glob pattern when no paths are provided.
test/utils.py Updates test helper to use new default constants from cli.py.
test/test_ls.py Adds coverage for ls with default and custom output-prefix globbing.
test/duct_main/test_report.py Updates imports to new summary format constant location.
test/duct_main/test_aggregation.py Updates imports to new summary format constant location.
Comments suppressed due to low confidence (1)

src/con_duct/cli.py:287

  • The help text for --output-prefix lists {datetime} as a supported format variable, but LogPaths.create() only provides pid and datetime_filesafe when calling output_prefix.format(...). Using {datetime} will raise a KeyError at runtime. Either add a datetime value to LogPaths.create() formatting, or remove {datetime} from the help/ docs to match actual behavior.
        help="File string format to be used as a prefix for the files -- the captured "
        "stdout and stderr and the resource usage logs. The understood variables are "
        "{datetime}, {datetime_filesafe}, and {pid}. "
        "Leading directories will be created if they do not exist. "

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

prefix = args.output_prefix[: args.output_prefix.index("{")]
except ValueError:
prefix = args.output_prefix
pattern = f"{prefix}*"
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

When no paths are provided, the glob pattern is built as f"{prefix}". If output_prefix starts with '{', prefix becomes an empty string and the pattern becomes "", which can unintentionally scan the entire CWD and match many unrelated files. Consider restricting the glob to info files (e.g., "*info.json") and/or handling the empty-prefix case explicitly to avoid overly broad globbing.

Suggested change
pattern = f"{prefix}*"
# Avoid overly broad globbing when prefix is empty (e.g., output_prefix starts with '{').
# In that case, restrict the search to likely info files.
if prefix:
pattern = f"{prefix}*"
else:
pattern = "*info.json"

Copilot uses AI. Check for mistakes.
@asmacdo asmacdo added the semver-minor Increment the minor version when merged label Jan 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move CLI defaults to cli.py and refactor ls.py parameter handling

1 participant