fix: harden the 0.17 release boundaries - #20
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughThe release updates the package to 0.17.1 and adds filesystem, media, validation, diagnostic, publication, documentation, and metadata hardening. ChangesSecurity hardening
Release records
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@samsarix_creative_spirals/filesystem.py`:
- Around line 16-19: Make filesystem checks non-raising during
verify_campaign_plan_handoff: update is_link_like in
samsarix_creative_spirals/filesystem.py:16-19 to handle the listed filesystem
and path errors, and guard the complete predicates at
samsarix_creative_spirals/handoff.py:542, :586, :596, and :666. Convert failures
into recorded HandoffIssue values rather than allowing verification to abort.
In `@samsarix_creative_spirals/media_package.py`:
- Around line 579-580: Update _descriptor_path to support macOS and other POSIX
systems by resolving descriptors with fcntl.F_GETPATH when /proc/self/fd is
unavailable, or raise an explicit ConfigError when verification cannot be
performed. Preserve the existing Linux and Windows behavior so the containment
check always runs or fails closed.
- Around line 563-578: Update the Windows-specific path logic to use
sys.platform == "win32" consistently, including the surrounding Windows check.
In the relevant descriptor-resolution function, bind GetFinalPathNameByHandleW
through a private ctypes.WinDLL instance and configure explicit argument types
and return type so descriptor handles remain correct on 64-bit systems.
In `@samsarix_creative_spirals/models.py`:
- Around line 84-87: Update ConfigError’s issue-bounding logic so inputs
exceeding MAX_CONFIG_ISSUES reserve one slot for the “additional validation
issues omitted” message, keeping self.issues at or below the configured limit.
Preserve all 200 issues for exactly-limit input, and add boundary tests covering
200 and 201 issues.
In `@samsarix_creative_spirals/publication.py`:
- Around line 398-404: Return and use the reconstructed value produced by
CampaignPlanPublication.from_dict in the publication validation flow, while
preserving the existing ConfigError handling, so
verify_campaign_plan_publication evaluates serialized data rather than a
potentially divergent subclass instance. In tests/test_publication.py lines
430-451, add a regression using divergent in-memory and serialized values and
assert verification uses the reconstructed serialized publication.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 190db6bc-6344-4936-a91b-37334abe2a80
📒 Files selected for processing (22)
.github/CODEOWNERSCHANGELOG.mdCITATION.cffREADME.mdROADMAP.mdSECURITY.mddocs/PRODUCTIZATION.mdsamsarix_creative_spirals/_version.pysamsarix_creative_spirals/cli.pysamsarix_creative_spirals/filesystem.pysamsarix_creative_spirals/handoff.pysamsarix_creative_spirals/media_package.pysamsarix_creative_spirals/models.pysamsarix_creative_spirals/plan_import.pysamsarix_creative_spirals/plans.pysamsarix_creative_spirals/publication.pysamsarix_creative_spirals/workflow.pytests/test_cli.pytests/test_filesystem.pytests/test_plan_import.pytests/test_public_api.pytests/test_publication.py
| try: | ||
| metadata = path.lstat() | ||
| except FileNotFoundError: | ||
| return False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate is_link_like call sites and the enclosing functions.
rg -nP -C4 '\bis_link_like\s*\(' --glob '*.py'
ast-grep outline samsarix_creative_spirals/handoff.py --items all --type functionRepository: Deathcharge/samsarix-creative-spirals
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked candidate files:"
git ls-files | grep -E '(^|/)(filesystem|handoff)\.py$' || true
for f in $(git ls-files | grep -E '(^|/)(filesystem|handoff)\.py$'); do
echo "===== $f ====="
rg -n -C8 'is_link_like|is_symlink|verify_campaign_plan_handoff|HandoffIssue|artifact-(missing|type-invalid)' "$f" || true
done
echo "===== precise function map ====="
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline samsarix_creative_spirals/handoff.py --items all --type function || true
else
echo "ast-grep unavailable"
fiRepository: Deathcharge/samsarix-creative-spirals
Length of output: 23210
🏁 Script executed:
#!/bin/bash
set -u
echo "===== filesystem.py ====="
cat -n samsarix_creative_spirals/filesystem.py | sed -n '1,50p'
echo "===== handoff verifier flow ====="
cat -n samsarix_creative_spirals/handoff.py | sed -n '534,615p'
cat -n samsarix_creative_spirals/handoff.py | sed -n '875,955p'
echo "===== project metadata and related tests ====="
for f in $(git ls-files | grep -E '(^|/)(pyproject\.toml|setup\.cfg|setup\.py|tox\.ini|pytest\.ini)$'); do
echo "--- $f"
cat -n "$f" | sed -n '1,180p'
done
rg -n -C4 'is_link_like|NotADirectoryError|PermissionError|ELOOP|ENAMETOOLONG|artifact-missing|artifact-type-invalid|verify_campaign_plan_handoff' --glob '*.py' --glob '*test*' .Repository: Deathcharge/samsarix-creative-spirals
Length of output: 50393
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
import errno
import os
import pathlib
import tempfile
print("Python:", __import__("sys").version.split()[0])
print("Path.is_symlink source:")
import inspect
print(inspect.getsource(pathlib.Path.is_symlink))
with tempfile.TemporaryDirectory() as td:
root = pathlib.Path(td)
cases = [
("missing", root / "missing"),
("non-directory-parent", root / "file" / "child"),
("embedded-null", pathlib.Path(str(root) + "/bad\0name")),
("name-too-long", root / ("x" * 5000)),
]
(root / "file").write_text("x")
loop_a = root / "loop-a"
loop_b = root / "loop-b"
try:
loop_a.symlink_to(loop_b.name)
loop_b.symlink_to(loop_a.name)
cases.append(("symlink-loop-child", loop_a / "child"))
except OSError as exc:
print("could not create symlink-loop:", type(exc).__name__, exc)
for name, path in cases:
print(f"\n{name}: {path!r}")
for operation in ("lstat", "is_symlink", "exists", "is_file", "is_dir"):
try:
value = getattr(path, operation)()
print(operation, "=>", value)
except BaseException as exc:
print(operation, "RAISES", type(exc).__name__, "errno=", getattr(exc, "errno", None), repr(str(exc)))
class RaisingPath(pathlib.PosixPath):
_flavour = pathlib.PosixPath._flavour
def __new__(cls, error):
obj = super().__new__(cls, "/synthetic")
obj.error = error
return obj
def lstat(self):
raise self.error
print("\nSynthetic lstat errors:")
for exc in [
PermissionError(errno.EACCES, "permission denied"),
OSError(errno.ELOOP, "too many symbolic links"),
OSError(errno.ENAMETOOLONG, "file name too long"),
ValueError("embedded null byte"),
]:
path = RaisingPath(exc)
try:
path.lstat()
except BaseException as caught:
print(type(exc).__name__, "lstat raises", type(caught).__name__)
try:
path.is_symlink()
except BaseException as caught:
print(type(exc).__name__, "is_symlink raises", type(caught).__name__)
else:
print(type(exc).__name__, "is_symlink returns normally")
PYRepository: Deathcharge/samsarix-creative-spirals
Length of output: 32288
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
import errno
import inspect
import pathlib
print("pathlib._ignore_error:")
print(inspect.getsource(pathlib._ignore_error))
for method_name in ("exists", "is_dir", "is_file", "is_symlink"):
print(f"Path.{method_name}:")
print(inspect.getsource(getattr(pathlib.Path, method_name)))
class RaisingPath(pathlib.PosixPath):
_flavour = pathlib.PosixPath._flavour
def __new__(cls, error):
obj = super().__new__(cls, "/synthetic")
obj.error = error
return obj
def lstat(self):
raise self.error
def stat(self):
raise self.error
for error in (
PermissionError(errno.EACCES, "permission denied"),
OSError(errno.ELOOP, "too many symbolic links"),
OSError(errno.ENAMETOOLONG, "file name too long"),
ValueError("embedded null byte"),
):
print(f"\n{type(error).__name__}, errno={getattr(error, 'errno', None)}")
path = RaisingPath(error)
for method_name in ("is_symlink", "exists", "is_dir", "is_file"):
try:
result = getattr(path, method_name)()
except BaseException as caught:
print(method_name, "RAISES", type(caught).__name__)
else:
print(method_name, "=>", result)
PYRepository: Deathcharge/samsarix-creative-spirals
Length of output: 2689
Use a non-raising filesystem check throughout handoff verification. is_link_like() catches NotADirectoryError, but it still propagates PermissionError, ELOOP, ENAMETOOLONG, and ValueError. Path.exists() and Path.is_dir() can also propagate PermissionError and ENAMETOOLONG, so catching errors only in is_link_like() is insufficient. Guard the complete predicates at handoff.py#L542, handoff.py#L586, handoff.py#L596, and handoff.py#L666 so verify_campaign_plan_handoff records HandoffIssue values instead of aborting. The NotADirectoryError example no longer reproduces because the helper already catches it.
📍 Affects 2 files
samsarix_creative_spirals/filesystem.py#L16-L19(this comment)samsarix_creative_spirals/handoff.py#L596-L596samsarix_creative_spirals/handoff.py#L586-L586samsarix_creative_spirals/handoff.py#L542-L542
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@samsarix_creative_spirals/filesystem.py` around lines 16 - 19, Make
filesystem checks non-raising during verify_campaign_plan_handoff: update
is_link_like in samsarix_creative_spirals/filesystem.py:16-19 to handle the
listed filesystem and path errors, and guard the complete predicates at
samsarix_creative_spirals/handoff.py:542, :586, :596, and :666. Convert failures
into recorded HandoffIssue values rather than allowing verification to abort.
| if os.name == "nt": | ||
| import ctypes | ||
| import msvcrt | ||
|
|
||
| buffer = ctypes.create_unicode_buffer(32768) | ||
| length = ctypes.windll.kernel32.GetFinalPathNameByHandleW( | ||
| msvcrt.get_osfhandle(descriptor), buffer, len(buffer), 0 | ||
| ) | ||
| if not 0 < length < len(buffer): | ||
| return None | ||
| value = buffer.value | ||
| if value.startswith("\\\\?\\UNC\\"): | ||
| value = "\\\\" + value[8:] | ||
| elif value.startswith("\\\\?\\"): | ||
| value = value[4:] | ||
| return Path(value).resolve() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect mypy configuration for platform settings and per-module overrides.
fd -H -t f -e toml -e cfg -e ini . -d 2 --exec rg -n -C5 'mypy|platform|ignore_missing_imports|\[\[tool.mypy.overrides\]\]' {} \;
rg -n 'type: *ignore|windll|msvcrt|sys\.platform' --glob '*.py'Repository: Deathcharge/samsarix-creative-spirals
Length of output: 640
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- mypy configuration ---'
sed -n '60,100p' pyproject.toml
printf '%s\n' '--- target function context ---'
sed -n '520,590p' samsarix_creative_spirals/media_package.py
printf '%s\n' '--- relevant imports and usages ---'
rg -n -C3 '^(import|from) |windll|msvcrt|sys\.platform|os\.name|GetFinalPathNameByHandleW' samsarix_creative_spirals/media_package.py
printf '%s\n' '--- available type-check tools ---'
command -v mypy || true
python3 - <<'PY'
import sys
print("python:", sys.version)
try:
import mypy
print("mypy:", mypy.__file__)
except Exception as exc:
print("mypy unavailable:", exc)
PYRepository: Deathcharge/samsarix-creative-spirals
Length of output: 5832
🌐 Web query:
mypy sys.platform os.name platform-specific typeshed ctypes.windll msvcrt narrowing
💡 Result:
Mypy provides specific mechanisms to handle platform-specific code and avoid errors when accessing modules or attributes unavailable on the host operating system [1][2]. Platform Narrowing Mypy natively understands sys.platform checks within if, elif, else statements and top-level assert statements [1][2]. By using these, Mypy can narrow the type-checking context to the specified platform, ignoring code paths that are not applicable to the targeted OS [1][2]. To use this feature effectively: - Use sys.platform == "win32" (for Windows), sys.platform == "darwin" (for macOS), or sys.platform.startswith("linux") (for Linux) [1][2]. - You must import sys and check sys.platform directly [3]. Using aliases (e.g., from sys import platform) or other checks like os.name is not supported for this narrowing and will not suppress Mypy errors [4][5][3]. - For file-level exclusion, you can use a top-level assert sys.platform == "win32" at the beginning of a file, which instructs Mypy to ignore the entire file if it is not running on the specified platform [1][2]. Global Configuration If you need to check your codebase as if it were running on a specific platform, use the --platform flag in the command line or the platform configuration option in your mypy configuration file [1][2][6]. For example, running with --platform win32 will treat your code as if it were on Windows [1][2]. Typeshed and Platform-Specific Modules The Typeshed library includes conditional type definitions for modules like ctypes and msvcrt [7][8]. These stubs often use if sys.platform == "win32": internally to define Windows-only attributes such as ctypes.windll or msvcrt functions [7]. When your code is type-checked (or configured via --platform), Mypy leverages these conditional stubs to correctly resolve platform-specific symbols [9]. If you encounter persistent errors with platform-specific imports that are not resolved by the mechanisms above, you can use # type: ignore[import-not-found] or similar error codes to suppress them, although platform narrowing via sys.platform is the recommended, idiomatic approach [10]. Top results: [1], [7], [2], [6]
Citations:
- 1: https://mypy.readthedocs.io/en/latest/common_issues.html?highlight=reveal_type
- 2: https://mypy.readthedocs.io/en/stable/common_issues.html
- 3: How sys.platform is imported matters when doing system platform checks python/mypy#10054
- 4: mypy does not recognize os.name python/mypy#13002
- 5: Spec: Version and platform checking underspecified python/typing#1732
- 6: https://github.com/python/mypy/blob/master/docs/source/config%5Ffile.rst
- 7: https://github.com/python/typeshed/blob/master/stdlib/ctypes/__init__.pyi
- 8: Complete msvcrt python/typeshed#4548
- 9: ctypes windows only elements should be typechecked on linux python/typeshed#2099
- 10: https://stackoverflow.com/questions/71915551/prevent-mypy-errors-in-platform-dependent-python-code
🌐 Web query:
Python ctypes WinDLL use_last_error argtypes restype windll cache default argument conversion 64-bit HANDLE
💡 Result:
In Python's ctypes, WinDLL objects cache loaded libraries and their function pointers [1][2]. Consequently, using the global loaders (like ctypes.windll.user32) is discouraged because multiple packages can conflict if they attempt to redefine attributes like argtypes or restype on the same cached function object [1][2]. To avoid these issues, it is recommended to instantiate a private WinDLL object or a custom LibraryLoader [1][2][3]. Regarding your specific configuration requirements: 1. use_last_error: When you initialize a WinDLL with use_last_error=True, ctypes enables a mechanism to securely access the Windows system error code [4][5][6]. In this mode, ctypes maintains a thread-local copy of the error code, and it automatically manages the exchange between the system's GetLastError/SetLastError values and the ctypes private copy before and after foreign function calls [4][5]. 2. argtypes and restype: By default, ctypes assumes foreign functions return a C int and accept no specific types (often leading to truncation or memory corruption) [7][1]. You must explicitly set the.restype and.argtypes attributes on every function object to ensure proper marshaling, especially for 64-bit Windows environments where pointer sizes differ from integers [7][8]. 3. 64-bit HANDLE: A Windows HANDLE is typically defined as a void pointer [9]. In ctypes, this is represented as ctypes.c_void_p [9]. If you do not explicitly set argtypes to use a pointer type like c_void_p, ctypes may default to interpreting arguments as C ints, which causes 64-bit pointers to be truncated to 32 bits, leading to crashes or incorrect behavior [7][1][8]. Always define HANDLE parameters as c_void_p (or the specific types in ctypes.wintypes) in your argtypes tuple [7][1][9]. To safely configure a library, use: import ctypes from ctypes import wintypes # Create a private instance to avoid global cache conflicts kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) # Define the function signature explicitly kernel32.SomeFunction.restype = wintypes.BOOL kernel32.SomeFunction.argtypes = [wintypes.HANDLE, ctypes.c_wchar_p] # Call the function result = kernel32.SomeFunction(my_handle, "some_string")
Citations:
- 1: https://mail.python.org/pipermail/python-list/2016-May/859002.html
- 2: ctypes.CDLL returns singleton objects, resulting in usage conflicts python/cpython#66742
- 3: https://bugs.python.org/issue22552
- 4: https://docs.python.org/3/library/ctypes.html
- 5: https://docs.python.org/3.11/library/ctypes.html
- 6: https://docs.python.org/3.10/library/ctypes.html?highlight=ctypes
- 7: https://runebook.dev/en/docs/python/library/ctypes/ctypes.WinDLL
- 8: https://www.thecodingforums.com/threads/ctypes-from_param-truncating-64-bit-pointers-to-32-bit.743787/
- 9: https://github.com/python/cpython/blob/1aa1803b3df2af9481628b8896babbd686d314ff/Lib/ctypes/wintypes.py
Use sys.platform == "win32" for both Windows checks.
The project uses strict = true, and mypy narrows sys.platform checks but not os.name. Bind GetFinalPathNameByHandleW through a private ctypes.WinDLL instance with explicit argtypes and restype to avoid shared loader state and 64-bit handle truncation.
🧰 Tools
🪛 GitHub Actions: CI / 0_quality (3.13).txt
[error] 568-568: mypy: Module has no attribute "windll" [attr-defined]. The 'python -m mypy' check failed.
[error] 569-569: mypy: Module has no attribute "get_osfhandle" [attr-defined]. The 'python -m mypy' check failed.
🪛 GitHub Actions: CI / 1_quality (3.10).txt
[error] 568-568: mypy: Module has no attribute "windll" [attr-defined]. The 'python -m mypy' command failed with exit code 1.
[error] 569-569: mypy: Module has no attribute "get_osfhandle" [attr-defined]. The 'python -m mypy' command failed with exit code 1.
🪛 GitHub Actions: CI / quality (3.10)
[error] 568-568: mypy: Module has no attribute "windll" [attr-defined]. Command 'python -m mypy' failed with exit code 1.
[error] 569-569: mypy: Module has no attribute "get_osfhandle" [attr-defined]. Command 'python -m mypy' failed with exit code 1.
🪛 GitHub Actions: CI / quality (3.13)
[error] 568-568: mypy error: Module has no attribute "windll" [attr-defined]. Command 'python -m mypy' failed.
[error] 569-569: mypy error: Module has no attribute "get_osfhandle" [attr-defined]. Command 'python -m mypy' failed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@samsarix_creative_spirals/media_package.py` around lines 563 - 578, Update
the Windows-specific path logic to use sys.platform == "win32" consistently,
including the surrounding Windows check. In the relevant descriptor-resolution
function, bind GetFinalPathNameByHandleW through a private ctypes.WinDLL
instance and configure explicit argument types and return type so descriptor
handles remain correct on 64-bit systems.
Source: Linters/SAST tools
| proc_path = Path(f"/proc/self/fd/{descriptor}") | ||
| return proc_path.resolve() if proc_path.exists() else None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The descriptor containment check is silently disabled on non-Linux POSIX systems.
/proc/self/fd exists on Linux only. On macOS and BSD _descriptor_path returns None, so the containment test at Line 599 never runs and no diagnostic is produced. Windows and Linux enforce the check; other POSIX systems do not.
Add the macOS equivalent, fcntl.fcntl(descriptor, fcntl.F_GETPATH, ...), or fail closed with an explicit ConfigError when the opened path cannot be verified on any platform.
🔒️ Proposed fix to resolve the descriptor path on macOS
proc_path = Path(f"/proc/self/fd/{descriptor}")
- return proc_path.resolve() if proc_path.exists() else None
+ if proc_path.exists():
+ return proc_path.resolve()
+ get_path = getattr(fcntl, "F_GETPATH", None)
+ if get_path is not None:
+ raw = fcntl.fcntl(descriptor, get_path, bytes(1024))
+ return Path(os.fsdecode(raw.rstrip(b"\x00"))).resolve()
+ return NoneImport fcntl at module scope inside a POSIX guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@samsarix_creative_spirals/media_package.py` around lines 579 - 580, Update
_descriptor_path to support macOS and other POSIX systems by resolving
descriptors with fcntl.F_GETPATH when /proc/self/fd is unavailable, or raise an
explicit ConfigError when verification cannot be performed. Preserve the
existing Linux and Windows behavior so the containment check always runs or
fails closed.
| bounded = tuple(issues[:MAX_CONFIG_ISSUES]) | ||
| if len(issues) > MAX_CONFIG_ISSUES: | ||
| bounded = (*bounded, "additional validation issues omitted") | ||
| self.issues = bounded |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep ConfigError.issues within the configured limit.
When issues contains 201 entries, Line 84 retains 200 entries and Line 86 appends one more entry. self.issues then contains 201 entries.
Reserve one slot for the omission message only when the input exceeds MAX_CONFIG_ISSUES. Add boundary tests for 200 and 201 input issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@samsarix_creative_spirals/models.py` around lines 84 - 87, Update
ConfigError’s issue-bounding logic so inputs exceeding MAX_CONFIG_ISSUES reserve
one slot for the “additional validation issues omitted” message, keeping
self.issues at or below the configured limit. Preserve all 200 issues for
exactly-limit input, and add boundary tests covering 200 and 201 issues.
| try: | ||
| CampaignPlanPublication.from_dict(publication.to_dict()) | ||
| except ConfigError: | ||
| raise | ||
| except (AttributeError, TypeError, ValueError) as error: | ||
| raise ConfigError(f"publication object is structurally invalid: {error}") from error | ||
| return publication |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return and use the reconstructed publication value.
Line 399 validates a new CampaignPlanPublication, but Line 404 discards it. The isinstance check permits subclasses. A subclass can serialize a valid ledger from to_dict() while retaining an invalid status in records. verify_campaign_plan_publication then reads the original record. An unknown status is not an integrity error and is not counted as pending or failed, so verification can incorrectly report completion.
samsarix_creative_spirals/publication.py#L398-L404: Return the value fromCampaignPlanPublication.from_dict(...).tests/test_publication.py#L430-L451: Add a regression with divergent in-memory and serialized values. Assert that verification uses the reconstructed serialized value.
Proposed fix
try:
- CampaignPlanPublication.from_dict(publication.to_dict())
+ return CampaignPlanPublication.from_dict(publication.to_dict())
except ConfigError:
raise
except (AttributeError, TypeError, ValueError) as error:
raise ConfigError(f"publication object is structurally invalid: {error}") from error
-return publication📝 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.
| try: | |
| CampaignPlanPublication.from_dict(publication.to_dict()) | |
| except ConfigError: | |
| raise | |
| except (AttributeError, TypeError, ValueError) as error: | |
| raise ConfigError(f"publication object is structurally invalid: {error}") from error | |
| return publication | |
| try: | |
| return CampaignPlanPublication.from_dict(publication.to_dict()) | |
| except ConfigError: | |
| raise | |
| except (AttributeError, TypeError, ValueError) as error: | |
| raise ConfigError(f"publication object is structurally invalid: {error}") from error |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 403-403: Avoid specifying long messages outside the exception class
(TRY003)
📍 Affects 2 files
samsarix_creative_spirals/publication.py#L398-L404(this comment)tests/test_publication.py#L430-L451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@samsarix_creative_spirals/publication.py` around lines 398 - 404, Return and
use the reconstructed value produced by CampaignPlanPublication.from_dict in the
publication validation flow, while preserving the existing ConfigError handling,
so verify_campaign_plan_publication evaluates serialized data rather than a
potentially divergent subclass instance. In tests/test_publication.py lines
430-451, add a regression using divergent in-memory and serialized values and
assert verification uses the reconstructed serialized publication.
1 participant
Outcome
Closes all seven findings from the sealed pre-release security scan before the first public tag. Advances the package to 0.17.1; 0.17.0 was never published.
Security and behavior
Verification