Correctness hardening: fix 8 deterministic-helper bugs + regression tests (v0.6.6)#125
Conversation
…ts (v0.6.6)
A correctness audit of the helpers (the audit that did not finish in the v0.6.3
round) found eight real bugs, each with a reproduction; all fixed and pinned with a
regression test (tests/test_v066_bugfixes.py). The missing tests are why they
shipped green. No new features.
- cross_account_access_validator: a Deny expressed via NotAction/NotResource/
NotPrincipal was silently dropped -> false "allow" from a security-decision tool.
Now evaluates the inverted forms and flags an open-question caveat.
- throttle_tuning_recommender.parse_size: decimal units parsed as binary (64MB ->
67108864). Now honours the 'i' (MiB=1024, MB=1000), matching the sibling helper.
- small_object_analyzer: "GLACIER IR"/"Glacier Instant Retrieval" misclassified as
plain GLACIER (40KB vs 128KB min-billable). Now normalises class spelling.
- migration_cost_estimator: CSV total_size_bytes="0" shadowed the GB/TB fallback
(presence check now); malformed stdin/missing file 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"); now permits delivery + flags
over-broad.
- lifecycle_rule_simulator: same-day transition+expiration now sorts with expiration
precedence (S3 semantics), avoiding false "wasted days".
- parse_sigv4_error: no longer mislabels a short canonical-request fragment's last
line as payload_hash (requires >= 6 lines).
Validation: all gates green; 219 pytest (+8) + 21 extension tests; 31/31 baselines
PASS. Version 0.6.5 -> 0.6.6.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QhkyBLeDtBTmvTdVBqgCAV
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01494b2082
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 |
There was a problem hiding this comment.
When a supplied IAM/bucket/KMS statement omits both Action and NotAction (for example an incomplete pasted policy), this now matches every requested action. AWS policy statements do not grant permissions without an action, so the validator can report a false allow (or false explicit deny) instead of ignoring the malformed statement, which is especially risky for this access-decision helper.
Useful? React with 👍 / 👎.
| # 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() != "" |
There was a problem hiding this comment.
For stdin JSON that includes optional placeholders like {"total_size_bytes": null, "total_size_gb": 500}, _present() returns true because str(None) is non-empty, so _resolve_total_bytes() calls float(None) before it can fall back to the GB/TB field. This regresses JSON inputs that use null for unused fields and produces a traceback rather than the intended estimate/error envelope.
Useful? React with 👍 / 👎.
2 participants
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 8 real bugs, each with a reproduction. All are fixed and pinned with a regression test in
tests/test_v066_bugfixes.py— the missing tests are exactly why these shipped green. No new features, no new complexity.Fixes (ordered by impact)
cross_account_access_validator— security-decision bug. ADenyexpressed viaNotAction/NotResource/NotPrincipalwas silently dropped (the matchers only readAction/Resource/Principal), producing a false allow. Now evaluates the inverted forms correctly —Deny NotAction s3:GetObjectblockss3:PutObjectand still allowss3:GetObject— and surfaces an open-question caveat for inverted clauses.throttle_tuning_recommender.parse_size— decimal units parsed as binary.64MB→67108864 (should be 64000000), contradicting the siblingmultipart_etag_calculator. Now honours thei(MiB=1024, MB=1000).small_object_analyzer— storage-class spelling. "GLACIER IR" / "Glacier Instant Retrieval" fell through to plain GLACIER (40 KB min-billable instead of 128 KB), under-reporting the penalty. Now normalises class spelling.migration_cost_estimator— two bugs. A CSVtotal_size_bytes="0"(truthy string) shadowed the 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— false "delivery blocked". A wildcardPrincipal:"*"Allow was reported as "no S3 grant"; it genuinely permits S3 delivery, so it now passes and is flagged over-broad.lifecycle_rule_simulator— same-day tiebreak. Transition+expiration on the same day now sorts with expiration precedence (S3 semantics), avoiding a false "wasted days" on a class never really entered.parse_sigv4_error— payload-hash mislabel. A short canonical-request fragment's last line is no longer reported aspayload_hash(requires a full ≥6-line canonical request).Validation
All gates green (skill_integrity, version_reference, routing_contract, provider_scope, no_hardcoded_pricing, reference_scope, repo_size); 219 pytest (+8 regression tests) + 21 extension tests; 31/31 baselines 100% PASS. Version 0.6.5 → 0.6.6.
🤖 Generated with Claude Code
Generated by Claude Code