Correctness hardening: fix 8 deterministic-helper bugs + regression tests (v0.6.6)

CHANGELOG.md

@@ -1,5 +1,39 @@
 # Changelog
 
+## 2026-06-21 — v0.6.6: Correctness hardening (helper bug-fix release)
+
+A focused bug-fix release. A correctness audit of the deterministic helpers (the
+audit that did not complete in the v0.6.3 round) found eight real bugs, each with a
+reproduction; all are fixed and pinned with a regression test (the missing tests are
+why they shipped green). No new features; no new complexity.
+
+- **`cross_account_access_validator` silently dropped a Deny expressed via
+  `NotAction`/`NotResource`/`NotPrincipal`** → false "allow" from a security-decision
+  tool (highest impact). Now evaluates the inverted forms correctly (Deny-NotAction
+  blocks the non-excluded action; the excluded action stays allowed) and surfaces an
+  open-question caveat for inverted clauses.
+- **`throttle_tuning_recommender.parse_size` parsed decimal units as binary**
+  (`64MB`→67108864 instead of 64000000), contradicting the sibling
+  `multipart_etag_calculator`. Fixed to honour the `i` (MiB=1024, MB=1000).
+- **`small_object_analyzer` misclassified "GLACIER IR" / "Glacier Instant Retrieval"
+  as plain GLACIER** (40 KB min-billable instead of 128 KB), under-reporting the
+  small-object penalty. Now normalises class spelling (space/hyphen/prose).
+- **`migration_cost_estimator`**: a CSV `total_size_bytes="0"` (truthy string)
+  shadowed the `total_size_gb`/`tb` fallback → 0-byte migration; now a presence/value
+  check. Malformed stdin JSON / missing CSV now emit the `{"ok":false}` envelope
+  instead of a traceback.
+- **`notification_target_policy_validator`**: a wildcard `Principal:"*"` Allow was
+  reported as "no S3 grant" (false "delivery blocked"); it now correctly permits
+  delivery and flags the grant as over-broad.
+- **`lifecycle_rule_simulator`**: same-day transition+expiration now sorts
+  deterministically with expiration precedence (S3 semantics), avoiding a false
+  "wasted days" on a class never really entered.
+- **`parse_sigv4_error`**: stopped mislabeling a short canonical-request fragment's
+  last line as `payload_hash` (requires a full ≥6-line canonical request).
+
+Validation: all gates green; 219 pytest (+8 regression tests) + 21 extension tests;
+31/31 baselines 100% PASS. Version 0.6.5 → 0.6.6.
+
 ## 2026-06-21 — v0.6.5: Thicken non-AWS provider depth
 
 Continues the multi-provider rebalancing (v0.6.4) by deepening the actual content a

docs/ARCHITECTURE.md

@@ -1,6 +1,6 @@
 # Architecture
 
-StorageOps v0.6.5 is a Pi Coding Agent extension and skill pack.
+StorageOps v0.6.6 is a Pi Coding Agent extension and skill pack.
 
 ## Components
 

docs/cli-reference.md

@@ -41,7 +41,7 @@ from the older local package. Deployment provenance is written to
 ## `storageops --version`
 
 ```text
-StorageOps v0.6.5  (pi: 0.78.0)
+StorageOps v0.6.6  (pi: 0.78.0)
   httpmon             : /root/.storageops/bin/httpmon
   api key             : api-key file
   independent install : yes  (~/.storageops/agent)

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "storageops"
-version = "0.6.5"
+version = "0.6.6"
 description = "StorageOps — S3-compatible object storage diagnostic toolkit. A Pi Coding Agent extension + skill pack."
 requires-python = ">=3.11"
 readme = "README.md"

skill-registry.yaml

@@ -1,4 +1,4 @@
-# StorageOps Skill Registry v0.6.5
+# StorageOps Skill Registry v0.6.6
 
 # Machine-readable metadata for all StorageOps skills.
 # Skills are auto-discovered by Pi from the skills/ directory.

skills/storageops-event-notification/scripts/notification_target_policy_validator.py

@@ -64,10 +64,22 @@ def _statements(policy: dict) -> list:
     return [s for s in _as_list(policy.get("Statement")) if isinstance(s, dict)]
 
 
-def _principal_is_s3(stmt: dict) -> bool:
+def _principal_is_wildcard(stmt: dict) -> bool:
     principal = stmt.get("Principal")
     if principal == "*":
-        return False
+        return True
+    if isinstance(principal, dict):
+        return "*" in _as_list(principal.get("AWS")) or "*" in _as_list(principal.get("Service"))
+    return False
+
+
+def _principal_is_s3(stmt: dict) -> bool:
+    # A wildcard principal ("*", {"AWS":"*"}) DOES allow s3.amazonaws.com to invoke
+    # the target — over-broad, but it permits delivery, so it must not be reported
+    # as "no S3 grant" (false "delivery blocked").
+    if _principal_is_wildcard(stmt):
+        return True
+    principal = stmt.get("Principal")
     if isinstance(principal, str):
         return principal == S3_SERVICE
     if isinstance(principal, dict):
@@ -190,7 +202,13 @@ def validate(policy: dict, target_type: Optional[str], bucket_arn: Optional[str]
 
     policy_ok = not missing
 
+    overbroad = policy_ok and any(_principal_is_wildcard(s) for s in action_stmts)
     if policy_ok:
+        wildcard_note = (
+            " Note: the grant uses a wildcard Principal (\"*\"), which permits delivery but is "
+            "over-broad — scope it to Principal Service s3.amazonaws.com with an aws:SourceArn "
+            "condition." if overbroad else ""
+        )
         summary = (
             f"Target resource policy permits S3 delivery: an Allow statement grants "
             f"{action} to Principal {S3_SERVICE}"
@@ -200,6 +218,7 @@ def validate(policy: dict, target_type: Optional[str], bucket_arn: Optional[str]
             "Policy is correct. If events still are not delivered, the target policy is not "
             "the cause — check the bucket notification rule (event type + prefix/suffix filter) "
             "with notification_config_analyzer.py, and confirm CloudTrail shows the event was emitted."
+            + wildcard_note
         )
     elif sourcearn_mismatch:
         summary = (

skills/storageops-lifecycle-cost/scripts/lifecycle_rule_simulator.py

@@ -203,7 +203,10 @@ def analyze(rules: list, age_days, avg_size, object_count, start_class: str) ->
                 "orphaned/incomplete multipart parts remain billable"
             )
 
-    events.sort(key=lambda e: e[0])
+    # Deterministic tiebreak: when a transition and an expiration land on the same
+    # day, S3 gives expiration precedence (the transition does not happen), so sort
+    # expire before transition rather than relying on insertion order.
+    events.sort(key=lambda e: (e[0], 0 if e[1] == "expire" else 1))
 
     # Walk the timeline to determine class residency windows. A class entered at
     # day D and left at day L bills for max(L-D, min_duration_days). If the

skills/storageops-lifecycle-cost/scripts/small_object_analyzer.py

@@ -5,7 +5,7 @@
 CSV columns (required header): key,size_bytes,storage_class
 Output: JSON {ok, summary, class_breakdown, recommendations, details}"""
 
-import argparse, csv, json, sys
+import argparse, csv, json, re, sys
 
 # Minimum billable object size (bytes) per storage class family
 _MIN_BILLABLE = {
@@ -23,7 +23,11 @@
 
 
 def _min_billable(storage_class: str) -> int:
-    upper = storage_class.upper()
+    # Normalise spelling so "GLACIER IR" / "Glacier Instant Retrieval" / "glacier-ir"
+    # all match GLACIER_IR rather than falling through to plain GLACIER.
+    upper = re.sub(r"[ \-]+", "_", storage_class.upper())
+    if "INSTANT" in upper and "GLACIER" in upper:
+        upper = "GLACIER_IR"
     for marker in sorted(_MIN_BILLABLE, key=len, reverse=True):
         if marker in upper:
             return _MIN_BILLABLE[marker]

skills/storageops-migration-sync/scripts/migration_cost_estimator.py

@@ -37,14 +37,27 @@
 }
 
 
+def _present(row: dict, key: str) -> bool:
+    # CSV cells arrive as strings, so a literal "0" is truthy; test presence and a
+    # non-empty value, not truthiness, otherwise total_size_bytes="0" wrongly
+    # shadows a total_size_gb fallback.
+    return str(row.get(key, "")).strip() != ""
+
+
 def _resolve_total_bytes(row: dict) -> float:
-    """Byte-priority: total_size_bytes > total_size_gb > total_size_tb."""
-    if row.get("total_size_bytes"):
+    """Byte-priority: total_size_bytes > total_size_gb > total_size_tb.
+
+    A present-but-zero byte value is only used when it is the sole size given;
+    otherwise fall through to the GB/TB columns.
+    """
+    if _present(row, "total_size_bytes") and float(row["total_size_bytes"]) > 0:
         return float(row["total_size_bytes"])
-    if row.get("total_size_gb"):
+    if _present(row, "total_size_gb"):
         return float(row["total_size_gb"]) * 1e9
-    if row.get("total_size_tb"):
+    if _present(row, "total_size_tb"):
         return float(row["total_size_tb"]) * 1e12
+    if _present(row, "total_size_bytes"):
+        return float(row["total_size_bytes"])  # explicit 0 with no other size given
     raise ValueError("missing one of: total_size_bytes, total_size_gb, total_size_tb")
 
 
@@ -168,18 +181,22 @@ def main() -> None:
     ap.add_argument("--pretty", action="store_true", help="Pretty-print JSON output")
     args = ap.parse_args()
 
-    inputs: list[dict] = []
-    if args.stdin:
-        inputs = _parse_stdin()
-    elif args.file:
-        inputs = _parse_csv(args.file)
-    else:
-        ap.print_help()
-        raise SystemExit("specify --file or --stdin")
+    indent = 2 if args.pretty else None
+    try:
+        if args.stdin:
+            inputs = _parse_stdin()
+        elif args.file:
+            inputs = _parse_csv(args.file)
+        else:
+            ap.print_help()
+            raise SystemExit("specify --file or --stdin")
+    except (json.JSONDecodeError, OSError) as exc:
+        # Bad/unreadable input must yield the JSON error envelope, not a traceback.
+        print(json.dumps({"ok": False, "error": f"could not read input: {exc}"}, indent=indent))
+        return
 
     results = [estimate(row) for row in inputs]
     out = results[0] if len(results) == 1 else results
-    indent = 2 if args.pretty else None
     print(json.dumps(out, indent=indent))
 
 

skills/storageops-performance-diagnosis/scripts/throttle_tuning_recommender.py

@@ -72,11 +72,11 @@ def parse_size(value: Optional[str]) -> Optional[int]:
         raise ValueError(f"unparseable size: {value!r}")
     num = float(m.group(1))
     unit = m.group(2).lower()
-    binary = bool(m.group(3))
-    factor = {"": 1, "k": 1, "m": 2, "g": 3, "t": 4}[unit]
-    base = 1024 if binary or unit else 1000
+    binary = bool(m.group(3))  # the 'i' in MiB/GiB selects 1024, not the unit letter
     if unit == "":
-        base = 1
+        return int(num)
+    factor = {"k": 1, "m": 2, "g": 3, "t": 4}[unit]
+    base = 1024 if binary else 1000
     return int(num * (base ** factor))
 
 

skills/storageops-s3-protocol-compatibility/scripts/parse_sigv4_error.py

@@ -90,7 +90,10 @@ def _canonical_summary(canonical_request: str) -> dict[str, Any]:
         summary["path"] = lines[1]
     if len(lines) > 2:
         summary["query"] = lines[2]
-    if len(lines) >= 2:
+    # The payload hash is the last line of a *complete* canonical request (method,
+    # path, query, headers, blank, signed-headers, hash = >= 6 lines). On a short
+    # fragment lines[-1] is the path/query, not the hash — don't mislabel it.
+    if len(lines) >= 6:
         summary["payload_hash"] = lines[-1]
     signed_headers_index = None
     for index, line in enumerate(lines):

skills/storageops-security-iam-policy/scripts/cross_account_access_validator.py

@@ -73,24 +73,28 @@ def _glob_match(pattern: str, value: str) -> bool:
     return re.match(regex, value) is not None
 
 
-def _action_matches(stmt_actions: Any, action: str) -> bool:
-    for a in _as_list(stmt_actions):
-        if _glob_match(str(a), action):
-            return True
-    return False
-
-
-def _resource_matches(stmt_resources: Any, resource: str) -> bool:
-    resources = _as_list(stmt_resources)
-    if not resources:
-        # Identity policies always carry Resource; resource policies may omit it
-        # (meaning "this resource"). Treat an absent Resource as a match.
-        return True
-    return any(_glob_match(str(r), resource) for r in resources)
-
-
-def _principal_matches(stmt_principal: Any, principal_arn: str) -> bool:
-    """Does a resource-policy Principal grant the given caller principal ARN?"""
+def _action_matches(stmt: dict, action: str) -> tuple[bool, bool]:
+    """(matches, used_not). Honour Action and the inverted NotAction form."""
+    if "Action" in stmt:
+        return any(_glob_match(str(a), action) for a in _as_list(stmt["Action"])), False
+    if "NotAction" in stmt:
+        return (not any(_glob_match(str(a), action) for a in _as_list(stmt["NotAction"]))), True
+    return True, False  # no action constraint
+
+
+def _resource_matches(stmt: dict, resource: str) -> tuple[bool, bool]:
+    """(matches, used_not). Honour Resource and the inverted NotResource form."""
+    if "Resource" in stmt:
+        return any(_glob_match(str(r), resource) for r in _as_list(stmt["Resource"])), False
+    if "NotResource" in stmt:
+        return (not any(_glob_match(str(r), resource) for r in _as_list(stmt["NotResource"]))), True
+    # Identity policies always carry Resource; resource policies may omit it
+    # (meaning "this resource"). Treat an absent Resource as a match.
+    return True, False
+
+
+def _principal_grants(stmt_principal: Any, principal_arn: str) -> bool:
+    """Does a resource-policy Principal value grant the given caller principal ARN?"""
     if stmt_principal == "*":
         return True
     values: List[str] = []
@@ -115,6 +119,16 @@ def _principal_matches(stmt_principal: Any, principal_arn: str) -> bool:
     return False
 
 
+def _principal_matches(stmt: dict, principal_arn: str) -> tuple[bool, bool]:
+    """(matches, used_not). Honour Principal and the inverted NotPrincipal form."""
+    if "Principal" in stmt:
+        return _principal_grants(stmt["Principal"], principal_arn), False
+    if "NotPrincipal" in stmt:
+        # A statement applies to every principal NOT listed in NotPrincipal.
+        return (not _principal_grants(stmt["NotPrincipal"], principal_arn)), True
+    return False, False  # resource-policy statement with no principal grants nobody
+
+
 def _has_condition(stmt: dict) -> bool:
     return isinstance(stmt.get("Condition"), dict) and bool(stmt.get("Condition"))
 
@@ -129,23 +143,33 @@ def evaluate_link(
     """Evaluate one policy: explicit Deny > explicit Allow > implicit Deny."""
     allow_hit: Optional[str] = None
     conditional = False
+    allow_not = False
     for i, stmt in enumerate(statements, start=1):
         effect = str(stmt.get("Effect", "")).lower()
-        if not _action_matches(stmt.get("Action"), action):
+        a_match, a_not = _action_matches(stmt, action)
+        if not a_match:
             continue
-        if not _resource_matches(stmt.get("Resource"), resource):
-            continue
-        if match_principal and not _principal_matches(stmt.get("Principal"), principal_arn or ""):
+        r_match, r_not = _resource_matches(stmt, resource)
+        if not r_match:
             continue
+        used_not = a_not or r_not
+        if match_principal:
+            p_match, p_not = _principal_matches(stmt, principal_arn or "")
+            if not p_match:
+                continue
+            used_not = used_not or p_not
         sid = stmt.get("Sid", f"statement#{i}")
         if effect == "deny":
-            return {"result": "explicit_deny", "matched_sid": sid, "conditional": _has_condition(stmt)}
+            return {"result": "explicit_deny", "matched_sid": sid,
+                    "conditional": _has_condition(stmt), "not_clause": used_not}
         if effect == "allow" and allow_hit is None:
             allow_hit = sid
             conditional = _has_condition(stmt)
+            allow_not = used_not
     if allow_hit is not None:
-        return {"result": "allow", "matched_sid": allow_hit, "conditional": conditional}
-    return {"result": "implicit_deny", "matched_sid": None, "conditional": False}
+        return {"result": "allow", "matched_sid": allow_hit, "conditional": conditional,
+                "not_clause": allow_not}
+    return {"result": "implicit_deny", "matched_sid": None, "conditional": False, "not_clause": False}
 
 
 def _kms_action_for(action: str) -> Optional[str]:
@@ -283,6 +307,12 @@ def validate(
             "One or more matched statements carry a Condition; this validator matches the "
             "statement but does not evaluate Condition values — confirm the request meets them."
         )
+    if any(l.get("not_clause") for l in links):
+        open_questions.append(
+            "A matched statement uses an inverted element (NotAction/NotResource/NotPrincipal); "
+            "these are evaluated as 'everything except the listed set' and are easy to get wrong — "
+            "double-check the intended effect."
+        )
     if cross_account and bucket_policy is not None and identity_policy is None:
         open_questions.append("Caller IAM policy was not supplied; the identity-side allow is assumed unknown.")