.pr_agent_accepted_suggestions

PR 272 (2026-07-08)

[maintainability] Duplicated atomic JSON write

Duplicated atomic JSON write

Atomic temp-file JSON writes are implemented separately in `DuploCache.set()` and `update_cooldown()`, which risks behavior drift (e.g., permissions, fsync, error handling) across call sites. Consolidate this repeated logic into a shared helper and reuse it consistently.

Issue description

Atomic JSON file writing (temp file + os.replace) is duplicated in multiple places, which increases maintenance burden and can lead to subtle inconsistency over time.

Issue Context

The PR introduced atomic writes in both the general cache writer and the auth cooldown writer; these should share a single helper to keep behavior consistent.

Fix Focus Areas

src/duplo_resource/cache.py[58-66]
src/duplocloud/authcooldown.py[275-281]

[reliability] Cooldown stuck on cache error

Cooldown stuck on cache error

In DuploAPI.request_token(), when owns_cooldown is true, a failure in cache.set() prevents clear_auth_cooldown() from running, leaving the cooldown file behind until it expires and potentially blocking other processes. The exception also bubbles, causing the command to fail even though a token was successfully obtained.

Issue description

DuploAPI.request_token() clears the auth cooldown only after successfully writing credentials to the cache. If cache.set() raises (filesystem permissions, disk full, partial/failed replace, etc.), the function exits with an exception and never clears the cooldown file. This can (a) make a successful browser auth look like a failure to the user, and (b) leave a stale cooldown file that delays/blocks other concurrent processes until the cooldown window expires.

Issue Context

The regression was introduced by moving token caching into the owns_cooldown success path, before clear_auth_cooldown().
DuploCache.set() performs multiple filesystem operations (os.makedirs, open, json.dump, os.replace) without handling exceptions; any of these can raise.

Fix Focus Areas

Ensure the cooldown is cleared whenever owns_cooldown is true, regardless of whether caching succeeds (use try/finally).
Treat cache write failures as non-fatal for returning the freshly obtained token (log a warning and continue).
src/duplocloud/client.py[105-137]
src/duplo_resource/cache.py[51-67]

PR 267 (2026-06-18)

[maintainability] Changelog entry is run-on

Changelog entry is run-on

The added CHANGELOG entry is a single long, run-on sentence without a final period, which reduces readability and professionalism in user-facing documentation. This can make release notes harder to scan and understand.

Issue description

The newly added CHANGELOG entry is overly long/run-on and lacks final punctuation, reducing readability and professionalism.

Issue Context

This is user-facing release documentation and should be concise and easy to scan.

Fix Focus Areas

CHANGELOG.md[18-18]

PR 266 (2026-06-18)

[reliability] Swallowed stop/start failures

Swallowed stop/start failures

tenant.start()/tenant.stop() now catch any DuploError per resource and only warn, so genuine failures (e.g., 503 connectivity, 500 backend errors) no longer abort the command and the CLI can exit 0 while leaving resources unstarted/unstopped.

Issue description

tenant.start()/tenant.stop() currently suppress all DuploError exceptions during per-resource stop/start, converting them into warnings and continuing. This unintentionally masks real failures that should still fail the command (and produce non-zero exit), while only the specific “ineligible for stopping/starting” case should be skipped.

Issue Context

The CLI entrypoint exits non-zero only when a DuploError propagates out of the command handler. By catching all DuploErrors in the loop, the command can exit 0 even if resources were not started/stopped due to genuine failures.

Fix Focus Areas

src/duplo_resource/tenant.py[521-536]
src/duplo_resource/tenant.py[583-598]

Suggested fix

In the except DuploError as e: block, only warning+continue for the known “ineligible” condition (e.g., e.code == 400 and message contains not eligible for stopping and starting).
For any other DuploError, re-raise (or accumulate and raise at end) so the command fails appropriately.
Consider adding a regression unit test asserting that a non-ineligible DuploError still aborts/propagates.

[correctness] Misleading `stop()` success message

Misleading `stop()` success message

`tenant.start()` and `tenant.stop()` now catch `DuploError`, log that resources are being skipped, and continue processing, but they still return messages claiming all resources were started/stopped successfully, which can misrepresent the actual outcome. This can violate the requirement that overall command outcome/exit behavior reflect skipped or ineligible resources while continuing the loop.

Issue description

tenant.start() and tenant.stop() continue past ineligible/errored resources by catching DuploError (good for resiliency), but they still return full-success messages claiming all resources were started/stopped, which becomes inaccurate when any resource is skipped. Update the returned payload/outcome so it reflects partial completion (e.g., completed with skips) while still continuing to process eligible resources.

Issue Context

Compliance requires the command to keep processing eligible resources while surfacing skipped/ineligible ones, and for the overall command outcome/exit behavior to reflect that skips occurred. The code already logs a warning like Skipping ... and continues, but automation and operators consuming the returned JSON can be misled because the response still claims everything succeeded.

Fix Focus Areas

src/duplo_resource/tenant.py[521-536]
src/duplo_resource/tenant.py[528-536]
src/duplo_resource/tenant.py[583-598]
src/duplo_resource/tenant.py[590-598]

PR 264 (2026-05-21)

[correctness] AI CLI dispatch broken

AI CLI dispatch broken

`DuploAI` no longer inherits `DuploResource` and does not implement `__call__`, so `DuploCtl.__call__` cannot execute it as a callable resource. This makes `duploctl ai create_ticket` / `duploctl ai send_message` fail at runtime with a TypeError before any command method runs.

Issue description

DuploCtl.__call__ expects every CLI-exposed resource instance to be callable (r(*args, **kwargs)). DuploAI was changed to a plain class (no DuploResource base and no custom __call__), so duploctl ai <command> will now raise TypeError: 'DuploAI' object is not callable.

Issue Context

The @Resource decorator injects tenant/client fields by wrapping __init__, but it does not inject the command-dispatch __call__ method.
Command dispatch for most resources is provided by DuploResource.__call__, which routes the first CLI arg to an @Command method.

Fix

Make DuploAI callable again while keeping it non-CRUD:

Option A (preferred, consistent with other non-CRUD resources like jit): change DuploAI to inherit from duplocloud.resource.DuploResource (not V2/V3) and call super().__init__(duplo) in __init__.
Option B: implement a __call__(self, cmd: str, *args, **kwargs) that mirrors DuploResource.__call__ + command() behavior. Add a unit test that fails if the resource is not callable from the CLI path (e.g., assert hasattr(DuploAI, "__call__") is insufficient; instantiate with a small fake duplo stub and assert callable(duplo.load("ai")) OR directly exercise DuploCtl.__call__("ai", "send_message", ...) with stubbed client).

Fix Focus Areas

src/duplo_resource/ai.py[7-20]
src/duplocloud/controller.py[291-321]
src/duplocloud/resource.py[15-52]

PR 263 (2026-05-21)

[reliability] `_send_message_streaming` uses requests.post

`_send_message_streaming` uses requests.post

The new streaming sendMessage path calls `requests.post(..., stream=True)` directly and only translates `Timeout`/`ConnectionError`, so other `requests` failures can leak as raw exceptions and bypass the shared client’s unified response validation. This breaks the requirement to route HTTP verbs/transports through the common request/exception-handling layer and makes streaming behavior inconsistent with non-streaming calls.

Issue description

_send_message_streaming() bypasses the shared DuploAPI HTTP client wrapper by calling requests.post(..., stream=True) directly, and it only converts Timeout/ConnectionError into DuploConnectionError; as a result, other requests network failures can leak as raw exceptions and streaming responses can bypass the centralized response validation/mapping used elsewhere.

Issue Context

Compliance requires consistent network exception translation across HTTP verbs and transports, with streaming requests still following the project’s unified request/exception handling pattern. The existing client wrapper (DuploAPI._request) centrally builds the portal URL, injects auth headers, applies timeout, converts any requests.exceptions.RequestException into DuploConnectionError, and normalizes error handling via _validate_response(); the streaming path should preserve these guarantees while still supporting stream=True and Accept: text/event-stream.

Fix Focus Areas

src/duplo_resource/ai.py[284-348]
src/duplocloud/client.py[75-90]
src/duplocloud/client.py[148-166]

[correctness] `_resolve_workspace_id` may return None

`_resolve_workspace_id` may return None

`_resolve_workspace_id()` returns `matches[0].get("id")` without validating presence/type, so a malformed/partial admin-data response can propagate `None` into the ticket URL/payload and fail later with unclear errors. This violates the requirement to validate optional/variable-shaped inputs early and raise a clear `DuploError`.

Issue description

_resolve_workspace_id() (and similarly _resolve_agent_id()) returns the id field without validating that it exists and is a valid string, which can lead to downstream malformed URLs/payloads and non-obvious failures.

Issue Context

Admin data endpoints may return unexpected/malformed shapes (missing id, items not a list, etc.). Per compliance, the code should validate shapes early and raise a clear DuploError.

Fix Focus Areas

src/duplo_resource/ai.py[19-57]

[reliability] Unconditional `.json()` on responses

Unconditional `.json()` on responses

Multiple new calls unconditionally invoke `.json()` on HTTP responses without guarding for empty/204 bodies, which can raise JSON parsing exceptions at runtime. This violates the requirement to safely handle JSON parsing for empty/204 responses.

Issue description

New helpdesk/workspace/agent/ticket requests call .json() unconditionally, which can fail on HTTP 204 or empty response bodies.

Issue Context

Compliance requires safe JSON parsing behavior for empty/204 responses (returning a safe empty object or handling it explicitly) rather than always calling .json().

Fix Focus Areas

src/duplo_resource/ai.py[32-35]
src/duplo_resource/ai.py[49-52]
src/duplo_resource/ai.py[153-156]
src/duplo_resource/ai.py[240-243]
src/duplo_resource/ai.py[282-282]

[correctness] Unescaped lookup parameters

Unescaped lookup parameters

Workspace/agent name resolution interpolates user input directly into the query string, so names containing spaces or characters like '&' can break the request or alter query parameters. This can make lookups fail or resolve unintended resources.

Issue description

_resolve_workspace_id() and _resolve_agent_id() build URLs like ...?filters[name]={workspace_name} / ...?filters[name]={agent_name} without URL-encoding the value. Because the Duplo client passes the constructed string directly into requests.request(url=f"{host}/{path}"), special characters in names can produce malformed URLs or unintended query parameters.

Issue Context

These values are user-controlled CLI inputs (--workspace_name, --agent_name). Encoding should happen client-side to ensure correct behavior for names with spaces and to prevent query-string parameter injection.

Fix Focus Areas

src/duplo_resource/ai.py[19-57]
src/duplocloud/client.py[75-83]

Suggested fix

Use urllib.parse.quote_plus() (or similar) on workspace_name / agent_name when embedding into the URL string, OR refactor the client to accept a params dict and let requests encode it.
(Optional hardening) Validate the resolved id is non-empty before returning it, and raise DuploError if missing.

[correctness] Agent name ambiguity

Agent name ambiguity

`_resolve_agent_id()` returns the first case-insensitive match and does not error when multiple agents have the same name, potentially selecting the wrong agent ID. `_resolve_workspace_id()` already guards against this ambiguity, but agent resolution does not.

Issue description

_resolve_agent_id() uses next(...) to pick the first matching agent by name and does not detect duplicate exact (case-insensitive) matches. If duplicates exist, ticket creation may silently assign to an unintended agent.

Issue Context

Workspace name resolution already implements a safer pattern: collect matches, error on 0, error on >1. Agent resolution should mirror that to avoid nondeterministic behavior.

Fix Focus Areas

src/duplo_resource/ai.py[35-57]

Suggested fix

Replace the next(...) selection with a matches = [...] list.
Raise DuploError when len(matches) == 0 or len(matches) > 1.
Return matches[0]["id"] (with a defensive check that id exists).

PR 251 (2026-04-02)

[correctness] Defaults overwrite on apply

Defaults overwrite on apply

When a flat service YAML body lacks `OtherDockerConfig` or `AgentPlatform`, `DuploService.update()` injects defaults (`"{}"` / `0`) and posts them to `ReplicationControllerChangeAll`, potentially overwriting existing service settings. This is triggered by `DuploResourceV2.apply()` passing the user body directly to `update()` without merging with the current service state.

Issue description

DuploService.update() now supports flat (non-Template-wrapped) bodies, but it also fills missing keys with hard-coded defaults (OtherDockerConfig="{}", AgentPlatform=0). When apply() calls update() with a partial YAML body, these defaults can overwrite existing service values.

Issue Context

DuploResourceV2.apply() passes the user body directly into update(); update() does not merge with the existing service object unless body is None.

Fix Focus Areas

Use existing service as fallback source (not hard-coded defaults):
src/duplo_resource/service.py[151-174]
Confirm apply() update path behavior:
src/duplocloud/resource.py[147-156]

Suggested fix approach

When body does not contain Template (flat YAML), load existing = self.find(name) (or use cached old if already fetched for --wait).
For keys required by ReplicationControllerChangeAll (e.g., AgentPlatform, OtherDockerConfig, and possibly AllocationTags), if they are missing from body, fill them from existing/existing["Template"] rather than defaulting to 0/"{}".
Only fall back to 0/"{}" if neither the input nor the existing service provides a value.

PR 243 (2026-03-17)

[correctness] Cached GET breaks logs

Cached GET breaks logs

DuploArgoClient.get is now decorated with cachetools.cachedmethod, which builds a cache key from args/**kwargs; DuploArgoWorkflow.logs passes a dict via params=..., causing a TypeError (unhashable type: 'dict') before any HTTP request is made. This makes `duploctl argo_wf logs` fail at runtime.

Issue description

DuploArgoClient.get() is decorated with @cachedmethod, but callers pass params as a dict (and stream=True) in DuploArgoWorkflow.logs(). cachetools.cachedmethod requires all args/kwargs used in the cache key to be hashable, so params=dict raises TypeError and breaks the logs command.

Issue Context

argo_wf logs uses client.get(..., stream=True, params={...}).
Caching streaming requests.Response objects is also unsafe because the caller closes the response after reading.

Fix Focus Areas

src/duplo_resource/argo_client.py[100-120]
(context) src/duplo_resource/argo_wf.py[258-280]

Suggested fix approach

Split get into:
a cached helper that only accepts (api_path, tenant_id) (no **kwargs)
an uncached path for any request that sets stream=True or provides params/other kwargs Example shape:

PR 240 (2026-03-16)

[reliability] `apply` catches all `DuploError`

`apply` catches all `DuploError`

`apply()` falls back to `create()` for any `DuploError` thrown by `find()`/`update()`, which can mask real failures like 403/500. Only the intended not-found condition (e.g., 404) should trigger create; other errors should be re-raised.

Issue description

aws_secret.apply() catches all DuploError exceptions and falls back to create(), which can hide authorization/server errors and perform unintended creates.

Issue Context

Compliance requires fallback behavior only for the intended condition (typically 404 Not Found). Other status codes must be re-raised.

Fix Focus Areas

src/duplo_resource/aws_secret.py[221-225]

PR 239 (2026-03-16)

[correctness] JSON Pointer parent misparsed

JSON Pointer parent misparsed

`update_otherdockerconfig` pre-creates missing parent arrays by treating the JSON Pointer prefix as a single top-level dict key, so nested `.../-` paths (and escaped segments) won’t have their real parent array created and `jsonpatch.apply_patch` will still conflict. This makes the new behavior inconsistent with the documented nested JSON Patch path support for this command.

Issue description

update_otherdockerconfig tries to support RFC6902 add operations that append via .../- by pre-creating the parent array, but it currently treats the JSON Pointer prefix as a single top-level key (e.g., additionalContainers/0/envFrom). This does not match how JSON Patch resolves JSON Pointer segments, so nested append paths will still fail.

Issue Context

Patch paths are normalized to JSON Pointer form (leading /) by JsonPatchAction.
Patches are applied with jsonpatch.apply_patch, which resolves JSON Pointer segments (including ~0/~1 unescaping).
The pre-create logic must therefore walk the pointer segments and create the array at the correct nested parent location.

Fix Focus Areas

src/duplo_resource/service.py[199-213]
src/duplocloud/argtype.py[133-154]
src/duplocloud/controller.py[369-385]

Implementation notes (non-exhaustive)

For each patch with destination path ending in /- and op that can append (add at minimum; consider copy/move destinations too if you want parity):
Split the pointer into segments (drop leading empty segment), unescape ~1 -&gt; / and ~0 -&gt; ~.
Traverse through docker_config using segments up to the parent container:
If current container is a dict: create missing dict keys as {} when you need to traverse deeper.
If current container is a list: require numeric index segments and bounds-check; do not fabricate list elements.
At the final parent key (the segment immediately before -): if missing, set to [].
If present but None, decide whether to treat as missing and set to [] or raise a clear DuploError.
If present and not a list, raise a clear DuploError explaining the expected array type.
Add a unit test in src/tests/test_service.py that covers:
Missing top-level key: /EnvFrom/-
Missing nested key under an existing object (e.g., /additionalContainers/0/envFrom/-) to validate the traversal logic.

PR 237 (2026-03-13)

[correctness] Apply misses 404 errors

Apply misses 404 errors

`DuploResourceV2.apply()`/`DuploResourceV3.apply()` now catch only `DuploNotFound`, so resources whose `find()` raises `DuploError(..., 404)` will cause `apply()` to error instead of falling back to `create()`. This breaks `apply` for resources like `batch_queue` (custom `find`) and `ecr` (delegates not-found to `cloud_resource.find()` which raises `DuploError(404)`).

Issue description

DuploResourceV2.apply() and DuploResourceV3.apply() now catch only DuploNotFound. Many resources still raise DuploError(..., 404) from custom find() implementations, so apply() no longer falls back to create() for them.

Issue Context

Examples include batch_queue.find() and cloud_resource.find() (used by ecr.find()), both raising DuploError(404).

Fix Focus Areas

src/duplocloud/resource.py[143-152]
src/duplocloud/resource.py[335-340]
src/duplo_resource/batch_queue.py[48-70]
src/duplo_resource/cloud_resource.py[63-95]
src/duplo_resource/batch_job.py[59-84]

Notes

Preferred approach: in both apply methods catch DuploError as e and only create when e.code == 404 (re-raise otherwise). Optionally, migrate custom find() methods to raise DuploNotFound(name, kind) for consistency.

[correctness] Infra update mis-compares

Infra update mis-compares

`DuploInfrastructure.update()` only compares keys that exist in the server response and uses shallow `!=` for values, so it can miss differences for body keys absent in `existing` and can falsely report changes for nested objects like `Vnet`. This contradicts the documented behavior “raises if any body field differs from existing state” and can raise 422 for normal inputs (e.g., YAML contains only `Vnet.SubnetCidr` while existing includes `Vnet.AddressPrefix`).

Issue description

infrastructure update immutability validation uses a shallow top-level comparison and ignores body keys not present in the existing server response. This both contradicts the documented behavior and can produce false 422s for nested objects like Vnet when the body is a partial dict.

Issue Context

Repo test data shows infrastructure YAML commonly specifies Vnet partially (e.g., only SubnetCidr), while integration tests imply the API response includes additional Vnet keys such as AddressPrefix.

Fix Focus Areas

src/duplo_resource/infrastructure.py[201-216]
CHANGELOG.md[12-12]
src/tests/data/infrastructure.yaml[1-7]

Notes

A robust fix is a recursive comparator:

For each key in body:
if both values are dicts, recurse on only the keys provided in the body dict
otherwise compare scalar/list values directly (optionally normalize list ordering if order is not meaningful)
Consider whether missing keys in existing should be treated as a diff; align behavior with the changelog text.

PR 236 (2026-03-12)

[correctness] Uses nonexistent duplo.client

Uses nonexistent duplo.client

DuploCloudResource.list calls self.duplo.client.get, but DuploCtl does not expose a .client attribute (resources are injected with self.client). Any runtime call to cloud_resource.list (and ecr.list/find which depend on it) will raise AttributeError.

Issue description

DuploCloudResource.list() calls self.duplo.client.get(...), but DuploCtl does not provide a .client attribute. In this codebase, the @Resource decorator injects an HTTP client onto the resource instance as self.client, and other resources consistently use self.client.*.

Issue Context

This bug breaks duploctl cloud_resource list/find and also breaks ecr.list/find because DuploECR delegates to cloud_resource.

Fix Focus Areas

src/duplo_resource/cloud_resource.py[52-54]
(Replace self.duplo.client.get(...) with self.client.get(...).)

PR 235 (2026-03-12)

[correctness] Invalid client attribute use

Invalid client attribute use

DuploCloudResource.list calls `self.duplo.client.get(...)`, but `DuploCtl` does not expose a `client` attribute (clients are injected into resources as `self.client`). This causes an AttributeError at runtime and breaks both `cloud_resource` and `ecr` list/find operations that depend on it.

Issue description

DuploCloudResource.list() calls self.duplo.client.get(...), but the HTTP client is injected onto resources as self.client. DuploCtl has no .client attribute, so this will raise AttributeError and break cloud_resource and ecr list/find.

Issue Context

Resources receive a self.client via _inject_client in duplocloud/commander.py.

Fix Focus Areas

src/duplo_resource/cloud_resource.py[52-55]

PR 233 (2026-03-12)

[reliability] Unit tests lack SDK dep

Unit tests lack SDK dep

`duplocloud-sdk` was moved out of default dependencies, but the unit-test workflow installs `.[build,test]` and the unit tests import `duplocloud_sdk` and require `load_model()` to return SDK models. This will raise `ImportError`/assert failures and break CI unit tests.

Issue description

duplocloud-sdk was moved to the optional [sdk] extra, but CI unit tests and dev tooling still install only .[build,test]. Unit tests import duplocloud_sdk and assert SDK models load, so CI will fail without installing the extra.

Issue Context

Homebrew/pip default installs should remain SDK-free, but unit tests (and likely local dev setup) need the SDK.

Fix Focus Areas

pyproject.toml[46-59]
.github/workflows/test_unit.yml[20-35]
scripts/setup.sh[17-19]

[correctness] Validate flag silently no-ops

Validate flag silently no-ops

When `--validate` is enabled but `duplocloud_sdk` isn’t installed, `DuploCtl.load_model()` returns `None` and `DuploResource.command()` skips validation because it only validates when `model` is truthy. This makes `--validate` claim validation but still sends unvalidated request bodies.

Issue description

With the SDK optional, --validate can be enabled while duplocloud_sdk is absent. Current logic returns None for the model and silently skips validation, violating the contract of --validate.

Issue Context

Only commands with a declared model need the SDK. For those commands, validation should either occur or explicitly error.

Fix Focus Areas

src/duplocloud/controller.py[439-477]
src/duplocloud/resource.py[37-51]
src/duplocloud/args.py[269-273]

PR 231 (2026-03-11)

[correctness] `name` shown as `None`

`name` shown as `None`

User-facing messages interpolate the raw `name` parameter, which may be `None` when the tenant is resolved via default/`-T`. This can confuse users and makes error/success output inconsistent with the actual tenant used.

Issue description

set_metadata() formats user-facing messages with the optional name parameter, which can be None when the tenant is resolved via defaults (e.g., -T). This produces inaccurate messages like tenant &#x27;None&#x27;.

Issue Context

The method already resolves the tenant via tenant = self.find(name). Use the resolved tenant identity for all user-facing strings.

Fix Focus Areas

src/duplo_resource/tenant.py[655-656]
src/duplo_resource/tenant.py[697-699]
src/duplo_resource/tenant.py[728-744]

[reliability] Mutable default `[]` args

Mutable default `[]` args

`set_metadata()` uses `[]` as default values for `metadata` and `deletes`, which can lead to shared-state bugs across calls. This violates the defensive-defaults requirement.

Issue description

set_metadata() uses mutable list literals as default argument values (metadata=[], deletes=[]), which can create shared state across invocations.

Issue Context

Use None defaults and normalize to empty lists inside the function body.

Fix Focus Areas

src/duplo_resource/tenant.py[655-658]
src/duplo_resource/tenant.py[697-709]
src/duplo_resource/tenant.py[726-727]

[correctness] Replace ordering broken

Replace ordering broken

DuploTenant.set_metadata always applies creations before deletions using a snapshot of existing metadata, so providing both delete+create for the same key in one call can delete the key (or raise a false 404) instead of replacing it. The docstring suggests “use --delete first”, but CLI flag ordering cannot change this behavior because the function logic is fixed.

Issue description

DuploTenant.set_metadata() snapshots current_map once and then processes creates before deletes, which breaks same-key replace flows (e.g., --delete k --metadata k text v) and can even raise a false 404 when a key is created earlier in the same invocation.

Issue Context

The function docstring says “Use --delete first if you need to replace a value”, but CLI argument ordering does not affect the function’s fixed loop order.

Fix Focus Areas

src/duplo_resource/tenant.py[699-740]
src/duplo_resource/tenant.py[661-665]

PR 229 (2026-03-11)

[reliability] Secret update validate crash

Secret update validate crash

With --validate enabled, secret.create and secret.update may be invoked without -f (name + literals/patches), but the new @Command(model=...) makes the wrapper validate body=None and fail before the secret methods (or the V3 base update) can default/fetch and patch the body.

Issue description

secret.create/update now declare a model=... on the @Command decorator. When --validate is enabled and the user does not pass -f/--cli-input, body is None, but the command wrapper still calls validate_model(model, None) and fails before the command can construct or fetch the body.

Issue Context

DuploSecret.create() explicitly supports body=None and builds body = {}.
DuploSecret.update() delegates to DuploResourceV3.update(), which explicitly supports body=None by fetching the existing resource and applying patches.

Fix Focus Areas

src/duplocloud/resource.py[37-51]
src/duplo_resource/secret.py[29-96]
src/duplo_resource/secret.py[98-176]
src/duplocloud/resource.py[282-308]
src/duplocloud/controller.py[451-473]

[correctness] Changelog entry grammar inconsistent

Changelog entry grammar inconsistent

The new changelog bullet uses inconsistent capitalization (`openapi`) and awkward phrasing, which is a user-facing documentation quality issue. This can confuse users and reduces documentation polish.

Issue description

The changelog entry added under Unreleased uses inconsistent capitalization (openapi) and awkward phrasing, which violates the requirement that user-facing documentation be correct and consistent.

Issue Context

This is a user-facing CHANGELOG.md entry. Improving grammar/capitalization reduces confusion and keeps documentation polished.

Fix Focus Areas

CHANGELOG.md[10-13]

PR 228 (2026-03-11)

[correctness] `apply()` ignores `wait`

`apply()` ignores `wait`

The new `hosts.apply` command accepts a `wait` argument but never uses it, so `--wait` has no effect and behavior diverges from expected CLI semantics. This can mislead users and break workflows that rely on waiting for host readiness.

Issue description

src/duplo_resource/hosts.py::apply() defines a wait parameter but never uses it, so duploctl hosts apply --wait does not actually wait.

Issue Context

Compliance requires aligning command semantics with user expectations; exposing a --wait option that is ignored is incorrect behavior.

Fix Focus Areas

src/duplo_resource/hosts.py[53-76]

[correctness] Tenant start/stop crash

Tenant start/stop crash

DuploHosts.name_from_body now returns None when FriendlyName is missing, but tenant.start/tenant.stop call service.name_from_body(item) and then invoke hosts.start/hosts.stop with that value (and an extra positional arg), which can raise TypeError/AttributeError at runtime instead of skipping unnamed hosts.

Issue description

DuploHosts.name_from_body() can now return None when a host list item has no FriendlyName. tenant start/tenant stop currently assumes service.name_from_body(item) is always a string and unconditionally calls service.start(...) / service.stop(...), which for hosts can crash (passing None as the name and also passing an extra positional arg).

Issue Context

Host list API may return items without FriendlyName.
Tenant workflows iterate all hosts and invoke lifecycle operations.

Fix Focus Areas

src/duplo_resource/tenant.py[498-504]
src/duplo_resource/tenant.py[553-559]
src/duplo_resource/hosts.py[309-316]

[reliability] Apply masks non-404 errors

Apply masks non-404 errors

DuploHosts.apply catches any DuploError from find and falls back to create, so auth/permission/server errors during list/find can incorrectly be treated as “not found,” triggering unwanted create attempts and hiding the real failure.

Issue description

hosts.apply() currently treats any DuploError thrown by hosts.find() as “host does not exist” and calls create(). This can incorrectly create resources (or attempt to) when the underlying failure is an auth/permission/server error.

Issue Context

DuploAPI raises DuploError for 401/403/400/5xx.
hosts.find() calls list() which can surface those errors.

Fix Focus Areas

src/duplo_resource/hosts.py[71-77]
src/duplocloud/client.py[148-164]

PR 224 (2026-03-09)

[reliability] `delete()` parses empty JSON

`delete()` parses empty JSON

`delete()` unconditionally calls `response.json()`, which can fail for 204/empty delete responses. This can cause runtime errors during normal delete operations.

Issue description

delete() paths for Argo workflow resources call .json() unconditionally. For typical delete semantics, servers may return 204 No Content or a 2xx with an empty body, which makes response.json() raise.

Issue Context

Compliance requires checking for 204/empty responses before JSON parsing.

Fix Focus Areas

src/duplo_resource/argo_wf.py[146-149]
src/duplo_resource/argo_wf.py[428-431]
src/duplo_resource/argo_client.py[144-164]

[correctness] Missing body crashes commands

Missing body crashes commands

`argo_wf` and `argo_wf_template` commands call `_ensure_namespace(body, ...)` assuming `body` is a dict; if the user omits `-f/--cli-input`, `args.BODY` remains `None` and the code raises `AttributeError` instead of a `DuploError`. This affects `create/apply` (workflows) and `create/update/apply` (templates).

Issue description

argo_wf / argo_wf_template commands crash with AttributeError when invoked without -f/--cli-input (or when the YAML root is not a mapping), because _ensure_namespace() assumes body is a dict and calls setdefault().

Issue Context

args.BODY is an optional flag and can be None.
YamlAction can also parse YAML into non-dict types.

Fix Focus Areas

src/duplo_resource/argo_wf.py[19-33]
src/duplo_resource/argo_wf.py[102-189]
src/duplo_resource/argo_wf.py[341-463]

Implementation notes

Add a guard in _ensure_namespace():
if not isinstance(body, dict): raise DuploError(&quot;Body is required; pass -f/--cli-input with a YAML/JSON object&quot;, 400)
optionally validate body.get(key) is dict when present.
(Optional) Add small unit tests for missing body behavior to ensure it raises DuploError instead of AttributeError.

PR 221 (2026-03-06)

[security] Env defaults may leak

Env defaults may leak

command_args_filter exports Arg.default, but Arg.default reads from environment variables; building docs with DUPLO_TOKEN/DUPLO_HOST/etc set could embed sensitive or environment-specific values into generated docs if templates render the default field.

Issue description

command_args_filter currently sets default: a.default for each argument. For env-backed args, Arg.default reads from the environment, which can make docs non-reproducible and may leak secrets (e.g., DUPLO_TOKEN) into generated documentation if templates render default.

Issue Context

Arg.default uses os.getenv(self.env, self.__default) when env is set.

Fix Focus Areas

scripts/mkdocs.py[200-214]
src/duplocloud/argtype.py[88-93]

Suggested remediation

In command_args_filter, avoid calling a.default for env-backed args. Options:
default = None if a.env else a.default
or retrieve the raw constructor default via a safe accessor (preferred): add a method/property on Arg like raw_default that returns self.__default without env resolution, then use that in docs.
Ensure templates use the env field to indicate environment configuration rather than rendering a resolved secret value.

PR 220 (2026-03-04)

[correctness] CHANGELOG text has typos

CHANGELOG text has typos

New release notes contain multiple typos/grammar issues in user-facing documentation. This can confuse users and reduces documentation quality.

Issue description

The newly added CHANGELOG entry contains multiple typos/grammar issues in user-facing release notes (e.g., introducses, entrypoiint, registrered).

Issue Context

Compliance requires documentation and user-facing strings to be correct and consistent to avoid confusing users.

Fix Focus Areas

CHANGELOG.md[20-24]

[correctness] build_command appends None token

build_command appends None token

`DuploCtl.build_command()` appends the raw `_token` value without validating/resolving it; when `_token` is missing this inserts `None` into the argv list and downstream code (e.g., JIT AWS credential_process generation) crashes when joining/executing the command.

Issue description

DuploCtl.build_command() currently appends self._token directly, which may be None. This can crash callers that join/execute the returned argv list (e.g., JIT’s AWS credential_process).

Issue Context

Token validation/resolution is now implemented on the injected HTTP client (DuploAPI.token), but build_command() bypasses it.

Fix Focus Areas

src/duplocloud/controller.py[502-535]
src/duplo_resource/jit.py[195-206]
src/duplocloud/client.py[19-27]

PR 219 (2026-03-03)

[correctness] `__call__` doc omits command

`__call__` doc omits command

The updated `DuploClient.__call__` docstring no longer documents the required command argument even though the implementation still expects the first positional item in `*args` to be the command. This can confuse users and lead to incorrect usage.

Issue description

DuploClient.__call__ still expects a command to be passed positionally (as the first element of *args), but the docstring no longer documents the command argument, making the documentation inconsistent with actual usage.

Issue Context

This PR changed __call__ to accept query and **kwargs, and removed the explicit command doc entry. Users still call this as client(resource, command, ...).

Fix Focus Areas

src/duplocloud/client.py[298-306]

PR 213 (2026-02-20)

[reliability] Non-GET calls leak requests

Non-GET calls leak requests

DuploClient only maps network exceptions to DuploConnectionError in get(); post/put/delete still call requests directly with no exception handling. Transient network failures during POST/PUT/DELETE (including the call that happens before entering a wait loop) will still raise raw requests exceptions and abort instead of becoming retryable/handleable DuploConnectionError.

Issue description

Only DuploClient.get() maps requests network exceptions to DuploConnectionError. post(), put(), and delete() still call requests.* without try/except, so transient DNS/TCP/timeout failures can escape as raw requests exceptions and bypass the intended error semantics.

Issue Context

The PR introduces DuploConnectionError and a wait() retry path for it, but that only helps if client methods reliably raise DuploConnectionError on network failures.

Fix Focus Areas

src/duplocloud/client.py[372-419]
src/duplocloud/client.py[347-370]

[security] `wait()` logs raw exception

`wait()` logs raw exception

The new `wait()` retry warning logs the full exception string, which may expose internal network/host details to end users. The log message is also unstructured, reducing auditability and making it harder to safely control what details are emitted.

Issue description

wait() logs the full exception string via {e} in a warning message. This can leak internal network/host details and is unstructured.

Issue Context

The wait loop intentionally retries on transient network failures, so the warning will be shown to users and may be emitted repeatedly. Per compliance, user-facing errors/logs should avoid internal details and prefer structured logging.

Fix Focus Areas

src/duplocloud/resource.py[71-73]

PR 211 (2026-02-17)

[correctness] Failing ecs update_image tests

Failing ecs update_image tests

`ecs update_image` now returns a different message when `--wait` is enabled, but existing unit tests still expect the old message even with `wait=True`, causing test failures and undermining coverage of the intended fix.

Issue description

DuploEcsService.update_image now sets the success message after the wait completes when --wait is enabled. Existing unit tests set mock_client.wait = True but still assert the old message (&quot;Updating a task definition and its corresponding service.&quot;), so they will fail.

Issue Context

The behavior change is correct for --wait, but the test expectations must be updated to match the new contract.

Fix Focus Areas

src/tests/test_ecs_service.py[8-75]

PR 210 (2026-02-17)

[correctness] apply wait flag unused

apply wait flag unused

`DuploResourceV3.apply()` still accepts/document a `wait` boolean, but after this change it no longer influences behavior (and never controlled waiting correctly). Waiting is governed only by `self.duplo.wait`, making `apply(wait=True)` misleading for API/CLI consumers.

Issue description

DuploResourceV3.apply() exposes a wait flag but does not use it; actual waiting is controlled by DuploClient.wait. This is misleading and makes apply(wait=True) a no-op.

Issue Context

This PR removed passing wait into create() (correctly), but left apply(wait=...) as a public parameter without behavior.

Fix Focus Areas

src/duplocloud/resource.py[294-324]
src/duplocloud/resource.py[227-263]

PR 207 (2026-02-11)

[security] 404 retry can double-prefix

404 retry can double-prefix

With the new `404` retry condition, `find()` will now retry even when the caller already supplied a fully-prefixed name; because `_prefix_name()` is not idempotent, the retry becomes a double-prefixed lookup and could return the wrong secret if such a secret exists (or at least adds an unnecessary second request).

Issue description

DuploAwsSecret.find() now retries on HTTP 404 by calling _prefix_name(name). If the user already passes a prefixed name (which the integration tests show is common), _prefix_name() will prepend another duploservices-&lt;tenant&gt; prefix and the retry can end up querying a different secret name.

Issue Context

This PR expanded retry conditions from only 400 to (400, 404).
_prefix_name() is not idempotent (no check for already-prefixed names).

Fix Focus Areas

src/duplo_resource/aws_secret.py[53-60]
src/duplo_resource/aws_secret.py[244-247]
src/tests/test_aws_secret.py[48-115]

Suggested implementation direction

Update _prefix_name() to return name unchanged if it is already prefixed (e.g., starts with duploservices-{self.duplo.tenant}- or duploservices-{self.duplo.tenant}/).
In find(), only perform the retry if _prefix_name(name) != name.
Add a unit test: when find() is called with an already-prefixed name and the API returns 404, it should not issue a second GET using a double-prefixed name.

PR 206 (2026-02-11)

[reliability] `name_from_body()` missing validation

`name_from_body()` missing validation

`name_from_body()` directly accesses `body["SecretName"]` without validating `body` shape, which can raise `KeyError` and break `apply()` on malformed/partial inputs. This violates the requirement to explicitly handle null/empty/missing fields in edge cases.

Issue description

name_from_body() can raise KeyError when SecretName is missing, which is an unhandled edge case for apply().

Issue Context

apply() depends on name_from_body(body); missing/invalid inputs should fail gracefully with a clear DuploError message.

Fix Focus Areas

src/duplo_resource/secret.py[21-22]
src/duplo_resource/secret.py[223-253]

PR 204 (2025-12-10)

[general] Avoid redundant function calls in loop

✅ Avoid redundant function calls in loop

To improve efficiency and readability, call get_pod_image(pod) once within the running_pod function, store its result in a local variable, and reuse that variable for comparisons and logging.

src/duplo_resource/service.py [955-960]

 # ignore this pod if the image is the old image
-if image_changed and get_pod_image(pod) != new_img:
-  self.duplo.logger.debug(f"IGNORING: Pod {pod['InstanceId']} is not on the new image, {new_img}. It is on {get_pod_image(pod)}")
-  return 0
-else:
-  self.duplo.logger.debug(f"CONTINUING: Pod {pod['InstanceId']} is running image {get_pod_image(pod)}, which matches desired image: {new_img}")
+if image_changed:
+  pod_image = get_pod_image(pod)
+  if pod_image != new_img:
+    self.duplo.logger.debug(f"IGNORING: Pod {pod['InstanceId']} is not on the new image, {new_img}. It is on {pod_image}")
+    return 0
+  else:
+    self.duplo.logger.debug(f"CONTINUING: Pod {pod['InstanceId']} is running image {pod_image}, which matches desired image: {new_img}")

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that get_pod_image(pod) is called multiple times, which is inefficient as it performs logging and string manipulation. Caching the result in a variable is a good practice for both performance and code clarity.

[learned best practice] Minimize loop debug logging

✅ Minimize loop debug logging

Reduce debug logs inside per-pod and per-iteration loops or guard them by log level/checkpoints; aggregate pod details and log once per iteration to minimize noise and cost.

src/duplo_resource/service.py [340-1007]

-self.duplo.logger.debug(f"UPDATE IMAGE")
-...
-self.duplo.logger.debug(f"Wait enabled, beginning wait process")
-...
-self.duplo.logger.debug(f"Retrieving raw image {raw_image} for {pod['InstanceId']}")
-...
-self.duplo.logger.debug(f"checking {pod['InstanceId']} for faults....")
-...
-else:
-  self.duplo.logger.debug(f"fault found for {f['Resource']['Name']}, did not match current pod {pod['InstanceId']}.")
-...
-self.duplo.logger.debug(f"IGNORING: Pod {pod['InstanceId']} is not on the new image, {new_img}. It is on {get_pod_image(pod)}")
-...
-self.duplo.logger.debug(f"CONTINUING: Pod {pod['InstanceId']} is running image {get_pod_image(pod)}, which matches desired image: {new_img}")
-...
+# Example: aggregate per-iteration logging
 self.duplo.logger.debug(f"Running wait check for {name}")
-...
-self.duplo.logger.debug(f"Service update completed, checking status of pods for {name}")
-...
+pods = self.pods(name)
+faults = self.tenant_svc.faults(id=self.tenant_id)
+
+pod_statuses = []
+for p in pods:
+  # minimal per-pod checks; avoid multiple debug lines
+  if image_changed:
+    pod_img = get_pod_image(p)
+    pod_statuses.append((p["InstanceId"], pod_img))
+if pod_statuses:
+  summary = ", ".join(f"{pid}:{img}" for pid, img in pod_statuses)
+  self.duplo.logger.debug(f"Pod images this iteration: {summary}")
+
+running = sum(running_pod(p, faults) for p in pods)
 self.duplo.logger.debug(f"Detected {running} running pods")
-...
-else:
-  self.duplo.logger.debug(f"Service {name} running/min: {running}/{min_replicas}.")

Suggestion importance[1-10]: 6

__

Why: Relevant best practice - Avoid overly verbose debug logging in tight loops; log succinctly and at appropriate levels to prevent log noise and performance issues.

PR 197 (2025-11-28)

[possible issue] Handle empty API responses gracefully

✅ Handle empty API responses gracefully

In the argo_request method, check for empty response bodies or a 204 status code before attempting to parse JSON to prevent a JSONDecodeError.

src/duplo_resource/argo_wf.py [174-177]

 if not (200 <= response.status_code < 300):
   raise DuploError(f"Argo API error: {response.text}", response.status_code)
 
+if response.status_code == 204 or not response.content:
+  return {}
 return response.json()

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that calling .json() on an empty response body, which is a common pattern for DELETE requests, would cause a runtime error and proposes a robust fix.

PR 191 (2025-10-31)

[general] Remove duplicated code block

✅ Remove duplicated code block

Remove the duplicated code block that checks for and assigns IpcMode to the result dictionary.

src/duplo_resource/ecs_service.py [297-300]

-if "IpcMode" in task_def:
-  result["IpcMode"] = task_def["IpcMode"]
 if "IpcMode" in task_def:
   result["IpcMode"] = task_def["IpcMode"]

Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a duplicated block of code. Removing the redundant code improves readability and maintainability, although it has no functional impact.

PR 183 (2025-09-16)

[general] Remove commented dead code

✅ Remove commented dead code

Remove the commented-out runs-on configuration for the self-hosted runner to eliminate dead code and improve the workflow file's clarity.

.github/workflows/installer.yml [135-139]

-# runs-on:
-# - self-hosted
-# - darwin
-# - arm64
 runs-on: macos-latest-xlarge

Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies commented-out code and recommends its removal, which is a valid code cleanup practice that improves maintainability.

PR 179 (2025-08-27)

[learned best practice] Extract image normalization helper method

✅ Extract image normalization helper method

Extract the image normalization logic into a shared helper method to avoid duplication and ensure consistent image processing across the codebase.

src/duplo_resource/service.py [710-716]

+def _normalize_image(self, image):
+  """Normalize image name by removing Docker registry prefixes."""
+  return image.removeprefix("docker.io/library/").removeprefix("docker.io/").removeprefix("library/")
+
 def get_pod_image(pod):
   """Get the image of a pod."""
-  raw_image=pod["Containers"][0]["Image"]
+  raw_image = pod["Containers"][0]["Image"]
   self.duplo.logger.debug(f"Retrieved raw image {raw_image} for {pod['InstanceId']}")
-  normalized_image = raw_image.removeprefix("docker.io/library/").removeprefix("docker.io/").removeprefix("library/")
+  normalized_image = self._normalize_image(raw_image)
   self.duplo.logger.debug(f"Returning normalized image {normalized_image} for {pod['InstanceId']}\n")
   return normalized_image

Suggestion importance[1-10]: 6

__

Why: Relevant best practice - Centralize and enforce consistent parameter and naming logic across methods by extracting shared helpers and aligning call sites to prevent duplication and mismatches.

PR 175 (2025-08-20)

[learned best practice] Correct duplicated and unclear docstring

✅ Correct duplicated and unclear docstring

Remove duplicated sentences and ensure consistent, concise phrasing. Adjust articles and capitalization in prose for clarity and correctness.

src/duplo_resource/service.py [626-653]

 """Start a service.
-
-Start a service.
 
 Usage: Basic CLI Use
   ```sh
   duploctl service start <service-name>
   duploctl service start --all
   duploctl service start --targets service1 service2 service3

-Example: Wait for Start

This command supports the global --wait. Waits for the desired count of pods to become ready. +Example: Wait for start

This command supports the global --wait flag. It waits for the desired number of pods to become ready.
```
duploctl service start myapp --wait --loglevel INFO
```

Args: name: The name of the service to start.

all: Boolean flag to start all services. Defaults to False.

all: Whether to start all services. Defaults to False. targets: List of service names to start. Cannot be used with name or all.

-Returns: +Returns: A summary containing services that were started successfully and those that encountered errors.

Raises: DuploError: If the service could not be started. """







Suggestion importance[1-10]: 6

__

Why: 
Relevant best practice - Fix grammatical errors and typos in documentation strings and comments to maintain professional code quality.

___

</details>

___





























































<!-- PR --><table><tr><td>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; <b><a href='https://github.com/duplocloud/duploctl/pull/174#issuecomment-3151430393'>PR 174</a></b> (2025-08-04)&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;

</td></tr></table>



<!-- suggestion --><details><summary>[learned best practice]  Fix typo in docstring</summary>

___

✅ Fix typo in docstring

**The word "reponse" is misspelled in the docstring return description. It should be "response" to maintain professional code quality and proper documentation standards.**

[src/duplo_resource/ai.py [57]](https://github.com/duplocloud/duploctl/pull/174/files#diff-64eea0ca92ec2729fd2d1abc7fd38b015249f57edb4849c0a2bec643c4046745R57-R57)

```diff
-ai_response: AI Agent reponse
+ai_response: AI Agent response

Suggestion importance[1-10]: 6

__

Why: Relevant best practice - Fix grammatical errors and typos in error messages, documentation strings, and comments to maintain professional code quality

PR 173 (2025-07-29)

[learned best practice] Fix typo in documentation

✅ Fix typo in documentation

The word "reponse" is misspelled and should be "response". Additionally, the ampersand (&) should be replaced with "and" for better readability in documentation.

src/duplo_resource/ai.py [53-54]

 Returns:
-    Ticket name, AI Agent reponse &  a Chat url.
+    Ticket name, AI Agent response and a Chat url.

Suggestion importance[1-10]: 6

__

Why: Relevant best practice - Fix grammatical errors and typos in error messages, documentation strings, and comments to maintain professional code quality

PR 156 (2025-05-29)

[general] Refactor duplicated prefix logic

✅ Refactor duplicated prefix logic

The prefix logic is duplicated in both find() and delete() methods. This could lead to inconsistencies if the prefix format changes in one place but not the other. Extract this logic into a helper method to ensure consistent behavior.

src/duplo_resource/aws_secret.py [51-54]

-# if the name has the prefix we good, otherwise add it
-prefix = f"duploservices-{self.duplo.tenant}-"
-if not name.startswith(prefix):
-  name = prefix + name
+# Use a helper method to ensure consistent prefix handling
+name = self._ensure_name_has_prefix(name)

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies code duplication between find() and delete() methods for prefix handling. While refactoring would improve maintainability, this is a minor code quality improvement rather than a critical issue.

PR 137 (2025-03-27)

[possible issue] Remove duplicate function definition

✅ Remove duplicate function definition

This function redefines the get_test_data function that's already imported at the top of the file. This duplication can lead to confusion about which function is being used. Remove this duplicate definition and use the imported function instead.

src/tests/test_cloudfront.py [23-27]

-def get_test_data(name) -> dict:
-  dir = pathlib.Path(__file__).parent.resolve()
-  f = f"{dir}/data/{name}.yaml"
-  with open(f, 'r') as stream:
-    return yaml.safe_load(stream)
+# Remove this duplicate function definition entirely, as it's already imported
+# from .conftest import get_test_data

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a significant issue - the test file defines a duplicate get_test_data function that shadows the imported function. This could lead to confusion and maintenance problems.

[general] Use or remove unused variable

✅ Use or remove unused variable

The elapsed_time variable is calculated but never used. This could lead to confusion and makes the code less maintainable. Consider either using this variable for logging or timeout checking, or removing it entirely.

src/duplo_resource/cloudfront.py [27-41]

 for attempt in range(max_attempts):
   try:
-    elapsed_time = time.time() - start_time
     status_response = self.find(distribution_id)
     status = status_response.get("Distribution", {}).get("Status", "Unknown").lower()
     if status != prev_status:
+      elapsed_time = time.time() - start_time
+      logging.info(f"CloudFront status for {distribution_id}: {status} (after {elapsed_time:.1f}s)")
       prev_status = status
     if status == "deployed":
       return status_response
     elif status in ["failed", "error"]:
       raise RuntimeError(f"CloudFront distribution {distribution_id} failed to deploy.")
   except Exception as e:
     logging.error(f"Error checking CloudFront status: {e}")
     raise
   time.sleep(60)

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the elapsed_time variable is calculated but never used in the wait_check method. The improved code makes good use of this variable by including it in a log message, which enhances debugging capabilities.

PR 135 (2025-03-24)

[possible issue] Fix inconsistent resource naming

✅ Fix inconsistent resource naming

**kwargs):

"""Helper function to execute a test and handle errors."""
try:
```
   return func(*args, **kwargs)
```
except DuploError as e:
```
   pytest.fail(f"Test failed: {e}")
```

class TestAsg:

 @pytest.mark.integration
 @pytest.mark.dependency(name="create_asg", scope="session")
 @pytest.mark.order(1)

def test_create_asg(self, duplo):
```
   r = duplo.load("asg")
```

def test_create_asg(self, asg_resource):

   r, asg_name = asg_resource
   body = {

```
       "FriendlyName": "duploctl",
```

       "FriendlyName": asg_name,
       "Zone": 1,
       "IsEbsOptimized": False,
       "DesiredCapacity": 1,

@@ -31,72 +46,46 @@ "IsClusterAutoscaled": True, "IsMinion": True }

```
   try:
```
```
       r.create(body, wait=True)
```
```
   except DuploError as e:
```

       pytest.fail(f"Failed to create ASG: {e}")

```
   execute_test(r.create, body, wait=True)
   time.sleep(60)
```
@pytest.mark.integration @pytest.mark.dependency(depends=["create_asg"], scope="session") @pytest.mark.order(2)

def test_find_asg(self, duplo):
```
   r = duplo.load("asg")
```
```
   tenant = r.tenant["AccountName"]
```
```
   try:
```

       asg = r.find(f"duploservices-{tenant}-duploctl")

```
   except DuploError as e:
```

       pytest.fail(f"Failed to find ASG: {e}")

   assert asg["FriendlyName"] == f"duploservices-{tenant}-duploctl"

def test_find_asg(self, asg_resource):
```
   r, asg_name = asg_resource
```

   asg = execute_test(r.find, asg_name)

```
   assert asg["FriendlyName"] == asg_name
```
@pytest.mark.integration @pytest.mark.dependency(depends=["create_asg"], scope="session") @pytest.mark.order(3)

def test_update_asg(self, duplo):
```
   r = duplo.load("asg")
```
```
   tenant = r.tenant["AccountName"]
```
```
   body = {
```

       "FriendlyName": f"duploservices-{tenant}-duploctl",

```
       "MinSize": 2,
```
```
       "MaxSize": 3
```
```
   }
```
```
   try:
```
```
       response = r.update(body)
```
```
   except DuploError as e:
```

       pytest.fail(f"Failed to update ASG: {e}")

def test_update_asg(self, asg_resource):
```
   r, asg_name = asg_resource
```

   body = {"FriendlyName": asg_name, "MinSize": 2, "MaxSize": 3}

```
   response = execute_test(r.update, body)
   assert "Successfully updated asg" in response["message"]
```
@pytest.mark.integration @pytest.mark.dependency(depends=["create_asg"], scope="session") @pytest.mark.order(4)

def test_list_asgs(self, duplo):
```
   r = duplo.load("asg")
```
```
   try:
```
```
       asgs = r.list()
```
```
   except DuploError as e:
```

       pytest.fail(f"Failed to list ASGs: {e}")

```
   assert isinstance(asgs, list)
```
```
   assert len(asgs) > 0
```

def test_list_asgs(self, asg_resource):
```
   r, _ = asg_resource
```
```
   asgs = execute_test(r.list)
```
```
   assert isinstance(asgs, list) and len(asgs) > 0
```
@pytest.mark.integration @pytest.mark.dependency(depends=["create_asg"], scope="session") @pytest.mark.order(5)

def test_scale_asg(self, duplo):
```
   r = duplo.load("asg")
```
```
   tenant = r.tenant["AccountName"]
```
```
   try:
```

       response = r.scale(f"duploservices-{tenant}-duploctl", min=1, max=2)

```
   except DuploError as e:
```

       pytest.fail(f"Failed to scale ASG: {e}")

def test_scale_asg(self, asg_resource):
```
   r, asg_name = asg_resource
```

   response = execute_test(r.scale, asg_name, min=1, max=2)
   assert "Successfully updated asg" in response["message"]

@pytest.mark.integration @pytest.mark.dependency(depends=["create_asg"], scope="session") @pytest.mark.order(6)

def test_delete_asg(self, duplo):
```
   r = duplo.load("asg")
```
```
   try:
```
```
       response = r.delete("duploctl")
```
```
   except DuploError as e:
```

       pytest.fail(f"Failed to delete ASG: {e}")

def test_delete_asg(self, asg_resource):
```
   r, _ = asg_resource
```

   response = execute_test(r.delete, "duploctl")
   assert "Successfully deleted asg" in response["message"]








**The delete method is using just "duploctl" as the ASG name, but other test methods use the full name with tenant prefix (e.g., "duploservices-{tenant}-duploctl"). This inconsistency could cause the test to fail if the API expects the full name.**

[src/tests/test_asg.py [93-102]](https://github.com/duplocloud/duploctl/pull/135/files#diff-a8011d44133b22fa8b9ed402ff058dc4469923ba774e2939b019502d43ae4a69R93-R102)

```diff
 @pytest.mark.integration
 @pytest.mark.dependency(depends=["create_asg"], scope="session")
 @pytest.mark.order(6)
 def test_delete_asg(self, duplo):
     r = duplo.load("asg")
+    tenant = r.tenant["AccountName"]
     try:
-        response = r.delete("duploctl")
+        response = r.delete(f"duploservices-{tenant}-duploctl")
     except DuploError as e:
         pytest.fail(f"Failed to delete ASG: {e}")
     assert "Successfully deleted asg" in response["message"]

Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical inconsistency in resource naming that would likely cause the test to fail. All other test methods use the full ASG name with tenant prefix, but the delete method uses just "duploctl", which is inconsistent and incorrect.

PR 132 (2025-03-20)

[possible issue] Fix parameter validation logic

✅ Fix parameter validation logic

The current condition is incorrect. It requires exactly one of the three parameters to be non-None, but doesn't properly handle the case when all three are None. Change the condition to explicitly check that at least one parameter is provided.

src/duplo_resource/service.py [249-250]

-if [image, container_image, init_container_image].count(None) != 2:
+if not any([image, container_image, init_container_image]):
   raise DuploError("Provide a service image, container images, or init container images.")
+if sum(x is not None for x in [image, container_image, init_container_image]) > 1:
+  raise DuploError("Provide only one of: service image, container images, or init container images.")

Suggestion importance[1-10]: 9

__

Why: The current validation logic is flawed. It checks if exactly one parameter is non-None, but fails to properly handle the case when all parameters are None. The improved code correctly validates that exactly one parameter is provided, preventing potential runtime errors.

PR 130 (2025-03-18)

[possible issue] Fix wait method behavior

✅ Fix wait method behavior

The wait_on_task method raises an error when tasks are still running, but this is unexpected behavior for a wait method. Instead, it should return a boolean indicating whether tasks are still running, which would allow the wait loop to continue properly.

src/duplo_resource/ecs_service.py [266-274]

 @Command()
 def wait_on_task(self,
-                 name: args.NAME) -> None: 
-  """Wait for an ECS task to complete."""
+                 name: args.NAME) -> bool: 
+  """Wait for an ECS task to complete.
+  
+  Returns:
+    bool: True if tasks are complete, False if still running
+  """
   tasks = self.list_tasks(name)
   # filter the tasks down to any where the DesiredStatus and LastStatus are different
   running_tasks = [t for t in tasks if t["DesiredStatus"] != t["LastStatus"]]
-  if len(running_tasks) > 0:
-    raise DuploError(f"Service {name} waiting for replicas update", 400)
+  return len(running_tasks) == 0

Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical logical issue in the wait_on_task method. The current implementation raises an error when tasks are still running, which breaks the expected behavior of a wait method. The fix properly changes the return type and behavior to support the wait loop pattern.

[general] Fix grammatical error

✅ Fix grammatical error

Fix the grammatical error in the message string. "it's" is the contraction of "it is", but the possessive form "its" is needed here. This improves code readability and professionalism.

src/duplo_resource/ecs_service.py [182]

 def update_image(self,
                  name: args.NAME,
                  image: args.IMAGE):
   """Update the image for an ECS service.
 
   Args:
     name: The name of the ECS service to update.
     image: The new image to use.
   Returns:
     ecs: The updated ECS object.
   Raises:
       DuploError: If the ECS service could not be updated.
   """
   name = self.prefixed_name(name)
   tdf = self.find_def(name) 
   tdf["ContainerDefinitions"][0]["Image"] = image
   arn = self.update_taskdef(tdf)["arn"]
-  msg = "Updating a task definition and it's corresponding service."
+  msg = "Updating a task definition and its corresponding service."

Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies a grammatical error ("it's" vs "its") in a message string. While this is a minor issue that doesn't affect functionality, fixing it improves code readability and professionalism.

PR 121 (2025-01-15)

[general] Fix grammatical error in failure message to maintain professional quality

✅ Fix grammatical error in failure message to maintain professional quality

Fix the typo in the error message where "exposed" should be "expose" to maintain consistent and professional error messages.

src/duplo_resource/service.py [713]

-"message": f"Failed to exposed service '{name}'",
+"message": f"Failed to expose service '{name}'",

Apply this suggestion Suggestion importance[1-10]: 3

Why: While the suggestion correctly identifies and fixes a grammatical error in the error message, it has minimal impact on functionality and is purely a cosmetic improvement.

PR 118 (2025-01-03)

[general] Fix incorrect resource name in documentation example

✅ Fix incorrect resource name in documentation example

The update method's docstring incorrectly refers to 'ssm_param' instead of 'aws_secret' in the CLI usage example.

src/duplo_resource/aws_secret.py [103]

 """Update a secret.
 Usage: cli usage
   ```sh
-  duploctl ssm_param update <name> -pval <newvalue>
+  duploctl aws_secret update <name> -pval <newvalue>


- [ ] **Apply this suggestion** 
Suggestion importance[1-10]: 5

Why: The suggestion fixes a documentation error that could confuse users, as it incorrectly references 'ssm_param' instead of 'aws_secret' in the CLI usage example.

___

</details>

___















































































<!-- PR --><table><tr><td>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; <b><a href='https://github.com/duplocloud/duploctl/pull/117#issuecomment-2565938696'>PR 117</a></b> (2024-12-30)&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;

</td></tr></table>



<!-- suggestion --><details><summary>[possible issue]  Initialize parameter values as strings to prevent type mismatches</summary>

___

✅ Initialize parameter values as strings to prevent type mismatches

**Initialize 'Value' as a string instead of an empty dictionary in the create method to prevent type errors.**

[src/duplo_resource/ssm_param.py [56-57]](https://github.com/duplocloud/duploctl/pull/117/files#diff-7889840699ca2d14591e807223704122ce333ecb84c25c4b826cfd87a00a70d0R56-R57)

```diff
 if 'Value' not in body:
-  body['Value'] = {}
+  body['Value'] = ""

Apply this suggestion Suggestion importance[1-10]: 8

Why: The suggestion fixes a potential type error by initializing Value as an empty string instead of an empty dictionary, which is more appropriate for SSM parameters and prevents runtime errors.

PR 113 (2024-11-08)

[Maintainability] Remove commented-out code that has been replaced by active code

✅ Remove commented-out code that has been replaced by active code

The commented-out code block should be removed since it's been replaced by a more efficient implementation using list comprehension.

src/duplo_resource/job.py [32-37]

 if fsct > 0:
   d = "\n".join([f["Description"] for f in fs])
   raise DuploFailedResource(f"Pod {pod['InstanceId']} raised {fsct} faults.\n{d}")
-# for f in faults:
-#   if f["Resource"].get("Name", None) == pod["InstanceId"]:
-#     raise DuploFailedResource(f"Pod {pod['InstanceId']} raised a fault.\n{f['Description']}")

Apply this suggestion Suggestion importance[1-10]: 5

Why: Removing commented-out code that's been replaced by a better implementation helps maintain cleaner, more maintainable code. The suggestion correctly identifies redundant code that should be removed.

Uh oh!

.pr_agent_accepted_suggestions

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Suggested fix

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Suggested fix

Issue description

Issue Context

Fix Focus Areas

Suggested fix

Issue description

Issue Context

Fix Focus Areas

Suggested fix approach

Issue description

Issue Context

Fix Focus Areas

Suggested fix approach

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Implementation notes (non-exhaustive)

Issue description

Issue Context

Fix Focus Areas

Notes

Issue description

Issue Context

Fix Focus Areas

Notes

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas

Issue description

Issue Context

Fix Focus Areas