Skip to content

Harden support and request security boundaries#439

Merged
binaryfire merged 25 commits into
0.4from
audit/support-and-request-security-correctness
Jul 17, 2026
Merged

Harden support and request security boundaries#439
binaryfire merged 25 commits into
0.4from
audit/support-and-request-security-correctness

Conversation

@binaryfire

@binaryfire binaryfire commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

This change hardens a set of foundational Support and request-security boundaries for Hypervel's long-lived worker model. It fixes concrete correctness, state-lifetime, secret-handling, and strict-typing failures across Support, authentication timing, encryption, hashing, and cookies while preserving Laravel-facing APIs and existing worker-level performance characteristics.

The main changes are:

  • publish generated providers, environment files, and Composer configuration atomically instead of truncating live files in place;
  • restore temporary global factories and test state deterministically when callbacks fail;
  • isolate mutable authentication timing state per authentication or password-reset operation;
  • correct Support utility, DTO, MIME, numeric, encoding, string, validated-input, and test-fake behavior;
  • harden encryption key rotation, AEAD failure contracts, key publication, secret redaction, and SerializableClosure global lifecycle state;
  • normalize strict hashing configuration, preserve custom-driver behavior, and redact plaintext throughout hashing call stacks;
  • make Cookie facade and jar retrieval contracts truthful for enum names, nested arrays, and mixed defaults;
  • document Hypervel's existing selective cookie-encryption mode and its security implications.

For more details, see: docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.md

Motivation

Several of these paths were inherited from request-per-process assumptions or had only been partially adapted to Hypervel's stricter types and long-lived Swoole workers. In that environment, temporary global mutation can survive failures, singleton-held mutable state can cross operation boundaries, and partial file writes can become the published application configuration.

The security packages also had narrower contract failures: rotated CBC keys were validated with observable short-circuit timing, invalid GCM tags could escape as native type errors, plaintext and application keys could remain in exception argument traces, custom hash drivers were bypassed by isHashed(), and Cookie APIs declared less than they could actually return.

This PR fixes those problems at their owning boundaries. It does not add a new lifecycle framework, registry, lock layer, request-wide cloning model, or compatibility shim.

Support and lifecycle correctness

Support file writers now use checked reads and mode-preserving atomic replacement. Generated provider files invalidate opcache only around successful publication, malformed Composer JSON fails explicitly, and interrupted writes cannot expose truncated configuration.

Temporary framework state is restored through exception-safe boundaries. This includes Lottery outcomes, Str random/UUID/ULID factories, facade and Dotenv lifecycle guidance, and authoritative test cleanup. Redundant cleanup that recreated non-coroutine context after it had been flushed is removed.

The Support utility corrections also cover:

  • callable normalization for encoded HTML;
  • current string and validated-input APIs;
  • file-size, pair-step, non-finite number, Base62, XML, JSON, and MIME edge cases;
  • DataObject reflection and nested dependency resolution;
  • Mail, Bus, Queue, exception-handler, and pending-batch fake parity and diagnostics.

Session guards and password brokers now clone their configured Timebox prototype for each timed operation. This keeps mutable early-return timing state isolated without changing ordinary authentication APIs or introducing request-scoped infrastructure.

Encryption and key lifecycle

CBC decryption validates every configured key MAC before selecting the first valid key, avoiding key-position timing disclosure during rotation. GCM payloads now reject missing or invalid tags through the framework's documented decryption exception.

Secret-bearing parameters are marked sensitive across contracts, implementations, helpers, providers, the Crypt facade boundary, and filesystem replacement. This prevents plaintext, application keys, and complete environment contents from being retained in exception traces while keeping unrelated facade diagnostics intact.

key:generate now uses checked filesystem reads and atomic replacement, matches exact quoted or unquoted key lines, preserves CRLF files, supports command prohibition, and updates runtime configuration only after publication succeeds.

Encryption boot now owns both setting and clearing the SerializableClosure signer. Test cleanup resets the signer and queue callback globals so stale keys and captured application graphs cannot survive between application lifecycles.

Hashing correctness

Bcrypt and Argon constructors normalize environment-shaped numeric and boolean configuration into their strict property types. Native password_hash() errors are translated into stable framework exceptions, and secret parameters are redacted through the complete hashing stack.

HashManager::isHashed() delegates to the configured cached driver instead of bypassing custom drivers. Verify-mode checks use the existing protected algorithm extension point, and facade metadata matches Hypervel's nullable passwordless contract.

Cookie retrieval and encryption guidance

Cookie facade reads now accept unit, string-backed, integer-backed, and zero-backed enum names. Mixed defaults are applied at the facade boundary without passing them into Request's narrower API, and existing cookie values still take precedence over supplied defaults.

CookieJar and QueueingFactory now truthfully expose nested cookie arrays and array defaults. Queued lookups truthfully support mixed sentinels, while conditional PHPStan types preserve precise no-default results. Queued-cookie collections advertise their Symfony Cookie element type end to end.

The response documentation now explains encryptCookies(only: [...]), that unlisted cookies remain plaintext, and that a non-empty only list takes precedence over except.

Compatibility and performance

Laravel-facing methods, configuration keys, and conventional extension points remain compatible. Additive APIs and behavioral corrections follow current Laravel where applicable; Hypervel-specific changes are limited to demonstrated Swoole, strict-typing, security, or existing-surface correctness needs.

The ordinary request path gains no new locks, registries, retries, service resolution, or retained worker state. Additional work is confined to the relevant operations:

  • one Timebox clone when an authentication or password-reset operation is timed;
  • complete CBC MAC validation only while previous keys are configured;
  • one cached driver lookup for hash classification;
  • enum normalization only when enum identifiers are supplied;
  • cold bootstrap, CLI, failure, documentation, and test paths elsewhere.

Testing

The changes include focused regressions for atomic publication failures, temporary-state restoration, DTO and utility edge cases, authentication timing isolation, encryption rotation and trace redaction, hashing custom-driver behavior, Cookie enum and nested-value handling, and selective-encryption precedence.

The full formatting, static-analysis, parallel test, Testbench contract, and Testbench dogfood gates pass.

Summary by CodeRabbit

  • New Features

    • Added Str::trans() / Str::counted() plus Stringable::counted() for count-aware pluralized strings.
    • Added ValidatedInput::file() for convenient uploaded-file retrieval.
    • Expanded cookie/facade support for enum keys and mixed/default cookie values.
    • Added new test assertions for queued/dispatch/batch behaviors (e.g., “pushed/queued” once & times).
  • Bug Fixes

    • Improved auth timing isolation by ensuring independent timeboxing per operation.
    • Hardened encryption, hashing, key generation, and config/env file handling; preserved permissions during updates.
    • Fixed multiple edge cases across utilities (numbers, MIME/XML, JSON, base62).
  • Documentation

    • Clarified auth routes omission, cookie selective-encryption precedence, and updated the “forever” cookie duration guidance.

Replace truncate-and-write updates with checked atomic replacement so readers never observe partial Composer or environment files. Preserve existing permissions, validate JSON and native I/O results, and invalidate the opcode cache only after publication succeeds.

Cover successful replacement, malformed input, write failures, permission preservation, and service-provider manifest publication with focused regressions.
Resolve constructor metadata without recursing into terminal built-in values, and handle untyped, union, nullable, defaulted, and custom dependency parameters at the owning reflection boundary. Cache empty dependency maps as valid results and preserve already-resolved nested data objects.

Expand the public guide and regressions to cover nested conversion, custom resolvers, defaults, nullability, collection inputs, and unsupported constructor shapes.
Treat the configured Timebox as a reusable prototype and clone it for each authentication or password-broker operation. This keeps early returns, exceptions, and overlapping coroutines from sharing mutable timing state while preserving custom Timebox subclasses and configured duration behavior.

Add concurrent and sequential regressions for session authentication and password validation paths, including short-circuit and exception exits.
Restore Lottery result factories through finally blocks so callbacks cannot strand test state, and make the test subscriber's non-coroutine fallback reset framework state without depending on a live request context. Remove redundant Once cleanup after the authoritative context teardown.

Add regressions for exceptional Lottery sequences and non-coroutine subscriber cleanup while preserving the existing ownership split between static reset and resource teardown.
Normalize closure binding at construction so static and instance callbacks follow one predictable execution path. Preserve valid callback scope while avoiding late binding failures when encoded HTML is rendered.

Cover static closures, object-bound closures, invokable callables, arguments, and repeated rendering through the public stringable contract.
Mark date factories, dotenv adapters, and facade application swaps with their real boot or test lifetime. The warnings identify the concrete cross-request risk of changing process-global or worker-shared configuration during request handling without altering the Laravel-facing APIs.
Reject invalid Base62 input without silently coercing it, preserve Number exponent keys across integer and string forms, escape JavaScript data safely, and make XML conversion handle invalid or unsupported values through explicit framework contracts.

Add focused regressions for boundary values, malformed input, escaping behavior, exponent lookup, and XML conversion failures.
Treat the native Fileinfo read as the authoritative boundary instead of trusting earlier path metadata. Convert disappeared or unreadable files into the documented guesser result and isolate the temporary error handler so coroutine-local warning handling cannot leak.

Exercise normal, missing, raced, warning, and non-coroutine paths with deterministic filesystem seams.
Add the current trans and counted helpers to Str and Stringable, and restore temporary random, UUID, and ULID factories through finally blocks when sequence callbacks fail. The public behavior follows current Laravel while keeping worker-lifetime test hooks from leaking after exceptional exits.

Document the new helpers and cover translation, pluralization, forwarding, and exceptional sequence cleanup.
Complete the current validated-input surface with file and hasFile accessors that resolve the active request at call time. This preserves coroutine isolation and gives validated input the same file lookup behavior applications expect from request data.

Document the API and cover single files, nested files, missing paths, invalid uploads, and request replacement.
Bring bus, mail, queue, pending-batch, and exception-handler fakes to the current public behavior, including sendNow routing, batch job inspection, pluralized count failures, strict fake matching, and clearer diagnostics. Keep fake callbacks and facade annotations aligned with their concrete APIs.

Expand the mail and queue guides and add regressions for pending operations, chain and batch inspection, job counts, serialization, exception restoration, and callback matching.
Document the upstream-deprecated auth facade aliases and Laravel's deferred-provider machinery as intentional omissions. Hypervel keeps the supported current APIs while avoiding compatibility-only surfaces and bootstrap optimizations that do not fit a long-lived Swoole worker.

Place the decisions where future upstream ports will see them without adding runtime compatibility shims.
Record the verified Support and authentication findings, rejected concerns, implementation results, validation, API impact, and final complexity assessment. Mark Support complete and route the next audit work to Encryption while preserving the auth revalidation dependency.
Validate every configured CBC key MAC before decrypting with the first valid key, eliminating key-position timing leakage during active rotation while retaining the existing GCM fallback flow.

Normalize missing and empty AEAD tags to the documented decryption exception, mark the concrete secret-bearing inputs as sensitive, and add focused regressions for complete MAC work and strict tag failures.
Apply SensitiveParameter metadata through the encryption contracts, atomic filesystem replacement, and the global encrypt helper so plaintext, keys, and environment contents do not remain in exception argument traces.

Keep the policy narrow by mirroring dynamic dispatch only in the Crypt facade rather than redacting every facade globally. Regression coverage pins both trace metadata and the inherited forwarding and missing-root behavior.
Publish generated application keys through checked, mode-preserving atomic filesystem replacement, match only the exact current quoted or unquoted key, preserve CRLF line endings, and update runtime configuration only after publication succeeds.

Add current command-prohibition behavior, make Encryption boot set or clear the SerializableClosure signer, and reset the signer, Queue callbacks, and command flag during authoritative test cleanup. Failure-injection and lifecycle regressions pin every commit and rollback boundary.
Cast application previous-key and Sanctum stateful-domain environment values at their configuration boundaries so explicit null values cannot escape into explode under strict typing.

Preserve each configuration's established filtered or unfiltered output shape and add isolated environment regressions with complete process-global restoration.
Record the verified encryption, filesystem, Foundation, Support, Testing, Queue, and Sanctum findings together with their final owning boundaries, rejected complexity, validation evidence, compatibility result, and approved CBC rotation cost.

Mark Encryption complete, preserve the outstanding owning-package revalidation routes, and advance the framework audit to Hashing with the enum-identifier dependency explicitly routed.
Normalize environment-shaped Bcrypt and Argon configuration before assigning strict properties, and translate native password_hash errors into the framework's stable exceptions.

Delegate hash detection to the configured driver and route algorithm checks through the existing protected extension seam. Add regressions for invalid configuration, custom drivers, numeric-string options, default cost, and algorithm overrides while preserving nullable and length-limit behavior.
Mark plaintext-bearing hashing parameters as sensitive across the contract, abstract implementation, helper, manager-facing facade path, and concrete call stack so exception traces cannot retain passwords.

Mirror the established Crypt facade dispatcher locally to redact packed arguments without weakening diagnostics for unrelated facades. Correct nullable facade metadata and add reflection plus forwarding regressions that pin the complete boundary and preserve base facade behavior.
Document the verified hashing defects, accepted boundaries, approved performance tradeoffs, rejected overengineering, implementation details, regression coverage, validation, and independent review outcome.

Mark Hashing complete, close its enum-manager revalidation, and route the framework audit to the Cookie package while keeping the package checklist synchronized with the source tree.
Allow the Cookie facade to resolve unit, backed, and integer-backed enum names at the request boundary while preserving all-cookie reads and mixed defaults.

Make CookieJar and QueueingFactory accurately describe nested cookie arrays, array defaults, mixed queued fallbacks, and queued Cookie element types. Correct the forever-cookie duration text to match the 400-day implementation.

Add focused regressions for enum zero identifiers, mixed default precedence, nested arrays, queued sentinels, and unchanged facade behavior.
Explain how applications can opt into encryption for only a named set of cookies through the existing middleware configuration API.

Call out that unlisted cookies remain plaintext and that a non-empty only list takes precedence over the except list, making the security consequences and runtime behavior explicit.
Capture the verified Cookie findings, rejected speculative changes, implementation boundary, regression coverage, validation, performance assessment, and owner approval in the audit ledger.

Mark Cookie complete and route the framework-wide audit to Engine with no pending cross-package revalidation.
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 879e36b3-0e43-4995-8b89-9ec145764787

📥 Commits

Reviewing files that changed from the base of the PR and between 30923ec and 52b47e0.

📒 Files selected for processing (8)
  • docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.md
  • src/encryption/src/EncryptionServiceProvider.php
  • src/support/src/DataObject.php
  • src/support/src/Js.php
  • tests/Hashing/HasherTest.php
  • tests/Support/DataObjectTest.php
  • tests/Support/SupportJsTest.php
  • tests/Support/SupportServiceProviderTest.php
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/support/src/Js.php
  • tests/Support/SupportServiceProviderTest.php
  • src/encryption/src/EncryptionServiceProvider.php
  • tests/Support/DataObjectTest.php
  • docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.md
  • src/support/src/DataObject.php

📝 Walkthrough

Walkthrough

This PR updates authentication isolation, encryption and hashing safeguards, cookie contracts, support utilities, testing fakes, lifecycle cleanup, audit metadata, documentation, and PHPUnit coverage for state restoration, typing, redaction, filesystem replacement, and edge cases.

Changes

Framework lifecycle and security audit

Layer / File(s) Summary
Audit ledger and package routing
docs/plans/*, src/auth/README.md, src/support/README.md
Audit records, dependency mappings, package checklists, and authentication documentation are updated.
Authentication Timebox isolation
src/auth/src/..., tests/Auth/*
Session guard and password broker operations use cloned Timebox instances, with tests covering repeated operations and sleep accounting.
Encryption and key publication
src/encryption/*, src/contracts/src/Encryption/*, src/filesystem/*, tests/Encryption/*, tests/Integration/Encryption/*
Encryption validates candidate MAC keys and AEAD tags, annotates secret parameters, resets serializable-closure state, and performs permission-aware environment replacement.
Hashing normalization and redaction
src/hashing/*, src/contracts/src/Hashing/*, src/foundation/*, tests/Hashing/*, tests/Foundation/*
Hasher options and error handling are normalized, configured-driver delegation is used for detection and algorithm checks, and plaintext parameters are redacted.
Cookie retrieval contracts
src/cookie/*, src/contracts/src/Cookie/*, src/support/src/Facades/Cookie.php, tests/Cookie/*
Cookie APIs support enum keys, mixed defaults, nested values, queued-cookie defaults, and updated return documentation.
Support runtime and validation
src/support/src/*, tests/Support/*, tests/Sanctum/*
Support utilities update data-object resolution, JSON and filesystem handling, numeric and string edge cases, MIME handling, worker-state restoration, XML handling, and uploaded-file access.
Testing fakes and lifecycle cleanup
src/support/src/Testing/*, src/testing/*, src/testbench/*, tests/Testing/*
Testing fakes gain exact-count assertions, job matching, synchronous mail sending, exception-reporting behavior, and expanded framework-state cleanup.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 69.03% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the PR’s main theme: hardening support and request-security boundaries across auth, encryption, hashing, cookies, and docs.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch audit/support-and-request-security-correctness

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown

Greptile Summary

This PR hardens a broad set of security and correctness boundaries across Support, encryption, hashing, auth timing, cookies, and the test lifecycle. The changes address concrete failures identified in a detailed audit of Hypervel's long-lived worker model: atomic file publication, exception-safe static-state restoration, per-operation Timebox isolation, CBC key-rotation timing, GCM tag validation, secret-parameter redaction, and custom-driver delegation in the hash manager.

  • Encryption: CBC key-rotation now validates all configured MACs before performing a single decryption, eliminating key-position timing disclosure. GCM payloads with missing or wrong-length tags are now correctly rejected via DecryptException. SensitiveParameter is propagated across contracts, implementations, facades, and the filesystem write path.
  • Hashing & auth: HashManager::isHashed() now delegates to the configured driver's info() instead of bypassing custom drivers. Timebox is cloned per authentication or password-reset operation, isolating mutable early-return state across coroutines.
  • Support / lifecycle: Lottery, Str UUID/ULID/random factories, and ServiceProvider bootstrap file writes are all hardened with try/finally. AfterEachTestSubscriber adds SerializableClosure global cleanup and removes the now-redundant Once::flushState() call.

Confidence Score: 5/5

Safe to merge. All core logic changes are correct and well-justified; the cipher, hashing, auth timing, and state-lifecycle fixes behave as described.

The CBC key-rotation fix is logically sound — all MACs are validated before a single decryption call, removing key-position timing leakage. The GCM null-tag fix, Timebox cloning, try/finally state restoration, isHashed() delegation, and atomic file writes are all correct. No logic errors, data-loss paths, or contract breaks were found.

The GCM decryption path in src/encryption/src/Encrypter.php (lines 146-168) retains sequential key-tryout timing that was eliminated for CBC — worth a follow-up if key rotation with multiple previous GCM keys is expected in production.

Important Files Changed

Filename Overview
src/encryption/src/Encrypter.php CBC key rotation timing hardened: MAC validation now exhausts all keys before any decryption; GCM null-tag rejection fixed; SensitiveParameter propagated to key-bearing methods.
src/encryption/src/Commands/KeyGenerateCommand.php Atomic env-file write with permission preservation; quoted/CRLF key regex fixed; Prohibitable support added; SensitiveParameter applied to key-bearing parameters.
src/auth/src/SessionGuard.php Timebox prototype cloned per timed operation (validate, attempt, attemptWhen) to isolate mutable early-return state across coroutines.
src/hashing/src/HashManager.php isHashed() now delegates to the configured driver's info() instead of bypassing custom drivers via password_get_info(); SensitiveParameter added.
src/hashing/src/BcryptHasher.php PHP 8 Error-based hashing failure handling; constructor normalises bool/int options; SensitiveParameter on plaintext; isUsingCorrectAlgorithm() extension point used in check().
src/encryption/src/EncryptionServiceProvider.php SerializableClosure key explicitly set to null when no app key exists, ensuring stale keys from previous boots do not persist; SensitiveParameter applied to key-parsing helpers.
src/support/src/Facades/Cookie.php get() and has() now accept UnitEnum keys; mixed defaults applied via ?? at facade boundary; return type broadened to mixed.
src/support/src/Lottery.php alwaysWin(), alwaysLose(), and forceResultWithSequence() now restore static factory state deterministically via try/finally when callbacks throw.
src/support/src/DataObject.php array_key_exists replaces falsy cache checks; class:: replaces static:: for customized-dependencies lookup in recursive resolution; DateTimeInterface detection uses is_a().
src/testing/src/PHPUnit/AfterEachTestSubscriber.php SerializableClosure global lifecycle state cleared; Once::flushState() removed (coroutine-scoped state is auto-cleaned); KeyGenerateCommand::flushState() added for Prohibitable static flag.

Reviews (2): Last reviewed commit: "Address support and security review find..." | Re-trigger Greptile

Comment thread src/support/src/Number.php
Comment thread src/support/src/ServiceProvider.php

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tests/Support/SupportServiceProviderTest.php (1)

437-465: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Move these file-writing cases to Testbench-managed temporary storage.

Both tests use a process-global temporary filename. Use a Testbench-based test with its disposable application skeleton or ParallelTesting::tempDir() to prevent collisions and unmanaged artifacts.

As per coding guidelines, “Use Testbench for container integration or file-writing tests” and use Testbench’s disposable skeleton or ParallelTesting::tempDir().

Also applies to: 468-497

🤖 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 `@tests/Support/SupportServiceProviderTest.php` around lines 437 - 465, Update
the file-writing tests around removeProviderFromBootstrapFile to use
Testbench-managed temporary storage, such as the disposable application skeleton
or ParallelTesting::tempDir(), instead of a process-global temporary filename.
Apply the same change to the additional cases around the referenced neighboring
test range, while preserving their existing assertions and cleanup behavior.

Source: Coding guidelines

src/support/src/Js.php (1)

88-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Pass JSON_INVALID_UTF8_SUBSTITUTE into Jsonable::toJson() src/support/src/Js.php:95 still bypasses the substitution flag on the Jsonable path, so malformed UTF-8 can throw for Jsonable implementations like Model and JsonResource instead of being replaced.

🤖 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 `@src/support/src/Js.php` around lines 88 - 102, Update the Jsonable branch in
Js::encode to pass JSON_INVALID_UTF8_SUBSTITUTE along with the existing required
flags to Jsonable::toJson(), ensuring malformed UTF-8 is replaced consistently
with the direct json_encode path.
🧹 Nitpick comments (2)
tests/Hashing/HasherTest.php (1)

267-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove unused local variable.

The $config variable assignment is unused and triggers a static analysis warning. You can pass the newly instantiated ConfigRepository directly.

♻️ Proposed refactor
         $container = m::mock(Container::class);
         $container->shouldReceive('make')
             ->with('config')
-            ->andReturn($config = new ConfigRepository([
+            ->andReturn(new ConfigRepository([
                 'hashing' => $hashing,
             ]));
🤖 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 `@tests/Hashing/HasherTest.php` around lines 267 - 272, Remove the unused
$config assignment in the container mock setup, while keeping the
ConfigRepository instance passed directly to andReturn() for the make('config')
expectation.

Source: Linters/SAST tools

src/support/src/Facades/Hash.php (1)

10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the isHashed method on the facade.

Consider adding @method static bool isHashed(string $value) to the docblock to reflect the public API exposed by HashManager, providing improved IDE auto-completion.

💡 Proposed change
  * `@method` static string make(string $value, array $options = [])
  * `@method` static bool check(string $value, string|null $hashedValue, array $options = [])
  * `@method` static bool needsRehash(string|null $hashedValue, array $options = [])
+ * `@method` static bool isHashed(string $value)
🤖 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 `@src/support/src/Facades/Hash.php` around lines 10 - 17, Add the missing
`@method static bool isHashed(string $value)` annotation to the `Hash` facade
docblock alongside the existing hashing methods, matching the public API exposed
by `HashManager`.
🤖 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 `@src/encryption/src/EncryptionServiceProvider.php`:
- Line 38: Type the $key parameter in the previous-key callback passed to
parseKey as string, preserving the existing SensitiveParameter attribute and
callback behavior.

In `@src/support/src/DataObject.php`:
- Around line 225-237: Update getDependencyFromUnionType to skip any union
member that is not a ReflectionNamedType before calling getName(), while
preserving the existing DataObject and DateTimeInterface matching behavior for
named members.

In `@tests/Support/SupportStringableTest.php`:
- Around line 128-136: Add the #[RequiresPhpExtension('intl')] attribute to the
testCounted() method in SupportStringableTest, matching the existing intl guard
used by related SupportStrTest coverage.

---

Outside diff comments:
In `@src/support/src/Js.php`:
- Around line 88-102: Update the Jsonable branch in Js::encode to pass
JSON_INVALID_UTF8_SUBSTITUTE along with the existing required flags to
Jsonable::toJson(), ensuring malformed UTF-8 is replaced consistently with the
direct json_encode path.

In `@tests/Support/SupportServiceProviderTest.php`:
- Around line 437-465: Update the file-writing tests around
removeProviderFromBootstrapFile to use Testbench-managed temporary storage, such
as the disposable application skeleton or ParallelTesting::tempDir(), instead of
a process-global temporary filename. Apply the same change to the additional
cases around the referenced neighboring test range, while preserving their
existing assertions and cleanup behavior.

---

Nitpick comments:
In `@src/support/src/Facades/Hash.php`:
- Around line 10-17: Add the missing `@method static bool isHashed(string
$value)` annotation to the `Hash` facade docblock alongside the existing hashing
methods, matching the public API exposed by `HashManager`.

In `@tests/Hashing/HasherTest.php`:
- Around line 267-272: Remove the unused $config assignment in the container
mock setup, while keeping the ConfigRepository instance passed directly to
andReturn() for the make('config') expectation.
🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 94c77835-3304-408d-af42-f732555187a9

📥 Commits

Reviewing files that changed from the base of the PR and between e56d998 and 30923ec.

📒 Files selected for processing (99)
  • docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.md
  • docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit.md
  • src/auth/README.md
  • src/auth/src/Passwords/PasswordBroker.php
  • src/auth/src/SessionGuard.php
  • src/boost/docs/data-objects.md
  • src/boost/docs/mail.md
  • src/boost/docs/queues.md
  • src/boost/docs/responses.md
  • src/boost/docs/strings.md
  • src/boost/docs/validation.md
  • src/contracts/src/Cookie/Factory.php
  • src/contracts/src/Cookie/QueueingFactory.php
  • src/contracts/src/Encryption/Encrypter.php
  • src/contracts/src/Encryption/StringEncrypter.php
  • src/contracts/src/Hashing/Hasher.php
  • src/cookie/src/CookieJar.php
  • src/encryption/composer.json
  • src/encryption/src/Commands/KeyGenerateCommand.php
  • src/encryption/src/Encrypter.php
  • src/encryption/src/EncryptionServiceProvider.php
  • src/filesystem/src/Filesystem.php
  • src/foundation/config/app.php
  • src/foundation/src/helpers.php
  • src/hashing/src/AbstractHasher.php
  • src/hashing/src/Argon2IdHasher.php
  • src/hashing/src/ArgonHasher.php
  • src/hashing/src/BcryptHasher.php
  • src/hashing/src/HashManager.php
  • src/sanctum/config/sanctum.php
  • src/support/README.md
  • src/support/src/Base62.php
  • src/support/src/Composer.php
  • src/support/src/DataObject.php
  • src/support/src/DateFactory.php
  • src/support/src/DotenvManager.php
  • src/support/src/EncodedHtmlString.php
  • src/support/src/Env.php
  • src/support/src/Facades/Auth.php
  • src/support/src/Facades/Bus.php
  • src/support/src/Facades/Cookie.php
  • src/support/src/Facades/Crypt.php
  • src/support/src/Facades/Facade.php
  • src/support/src/Facades/Hash.php
  • src/support/src/Facades/Mail.php
  • src/support/src/Facades/Queue.php
  • src/support/src/FileinfoMimeTypeGuesser.php
  • src/support/src/Js.php
  • src/support/src/Lottery.php
  • src/support/src/Number.php
  • src/support/src/Once.php
  • src/support/src/ServiceProvider.php
  • src/support/src/Str.php
  • src/support/src/Stringable.php
  • src/support/src/Testing/Fakes/BusFake.php
  • src/support/src/Testing/Fakes/ExceptionHandlerFake.php
  • src/support/src/Testing/Fakes/MailFake.php
  • src/support/src/Testing/Fakes/PendingBatchFake.php
  • src/support/src/Testing/Fakes/PendingMailFake.php
  • src/support/src/Testing/Fakes/QueueFake.php
  • src/support/src/ValidatedInput.php
  • src/support/src/Xml.php
  • src/testbench/src/Foundation/Application.php
  • src/testing/src/PHPUnit/AfterEachTestSubscriber.php
  • tests/Auth/TimeboxIsolationTest.php
  • tests/Cookie/CookieFacadeTest.php
  • tests/Cookie/CookieJarTest.php
  • tests/Encryption/CryptFacadeTest.php
  • tests/Encryption/EncrypterTest.php
  • tests/Encryption/SensitiveParameterTest.php
  • tests/Foundation/FoundationConfigTest.php
  • tests/Foundation/Testing/Concerns/InteractsWithExceptionHandlingTest.php
  • tests/Hashing/HashFacadeTest.php
  • tests/Hashing/HasherTest.php
  • tests/Hashing/SensitiveParameterTest.php
  • tests/Integration/Encryption/EncryptionTest.php
  • tests/Integration/Encryption/KeyGenerateCommandTest.php
  • tests/Integration/Queue/QueueFakeTest.php
  • tests/Sanctum/SanctumConfigTest.php
  • tests/Support/Base62Test.php
  • tests/Support/ComposerFileTest.php
  • tests/Support/DataObjectTest.php
  • tests/Support/EnvFileTest.php
  • tests/Support/FileinfoMimeTypeGuesserNonCoroutineTest.php
  • tests/Support/FileinfoMimeTypeGuesserTest.php
  • tests/Support/LotteryTest.php
  • tests/Support/NumberTest.php
  • tests/Support/PendingBatchFakeTest.php
  • tests/Support/SupportEncodedHtmlStringTest.php
  • tests/Support/SupportJsTest.php
  • tests/Support/SupportServiceProviderTest.php
  • tests/Support/SupportStrTest.php
  • tests/Support/SupportStringableTest.php
  • tests/Support/SupportTestingBusFakeTest.php
  • tests/Support/SupportTestingMailFakeTest.php
  • tests/Support/ValidatedInputTest.php
  • tests/Support/XmlTest.php
  • tests/Testing/PHPUnit/AfterEachTestSubscriberNonCoroutineTest.php
  • tests/Testing/PHPUnit/AfterEachTestSubscriberTest.php
💤 Files with no reviewable changes (2)
  • src/support/src/Once.php
  • src/testbench/src/Foundation/Application.php

Comment thread src/encryption/src/EncryptionServiceProvider.php Outdated
Comment thread src/support/src/DataObject.php
Comment thread tests/Support/SupportStringableTest.php
Complete invalid UTF-8 substitution for Jsonable values and make DataObject dependency reflection tolerate DNF intersection members without weakening supported dependency resolution.

Type previous encryption keys at their configuration boundary, move provider-file tests onto parallel-safe scratch directories, and remove a dead hashing test assignment.

Add focused regressions for both runtime defects and update the audit ledger to describe the complete corrected boundaries.
@binaryfire

Copy link
Copy Markdown
Collaborator Author

@greptileai @coderabbitai Thanks for the review. I checked every inline and main-thread item against the implementation, tests, and upstream behavior.

Addressed in 52b47e07a:

  • DataObject now skips intersection members while inspecting DNF unions. The regression uses an intersection-plus-DataObject declaration that previously failed before reaching the valid named member.
  • Js::encode() now applies invalid UTF-8 substitution to Jsonable values as well as the direct encoding path, with coverage for the missing branch.
  • The previous-encryption-key mapper now types its sensitive callback parameter as string.
  • Provider-file tests now use separate parallel-worker scratch directories with deterministic cleanup.
  • The unused hashing-test local was removed.
  • The audit ledger now records the complete DNF and JSON encoding boundaries.

The remaining suggestions were intentionally left unchanged:

  • Negative Number::pairs() steps remain supported. Laravel and Hypervel both normalize the magnitude with abs(), and both explicitly cover that behavior. Rejecting it would be an API divergence rather than a bug fix.
  • Both OPcache invalidations remain. The first protects the read in the read-modify-write sequence; the second invalidates the atomically replaced file for later loads.
  • The counted-string tests do not need an ext-intl attribute because the components root requires the extension and the suite cannot run without it. Current Laravel follows the same shape.
  • The Hash facade already declares isHashed() in its metadata, so no annotation change was necessary.

The focused regressions and the complete composer fix gate are green. The follow-up preserves the public API and adds no meaningful hot-path cost or new lifecycle machinery.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@binaryfire Thanks for the thorough validation and follow-up. The fixes in 52b47e07a address the reported DataObject, JSON encoding, key-mapper typing, test-isolation, cleanup, and audit-boundary issues.

The rationale for leaving the remaining items unchanged is sound: preserving negative-step compatibility in Number::pairs(), retaining both OPcache invalidations for their distinct safety boundaries, relying on the root ext-intl requirement, and keeping the already-present Hash::isHashed() metadata. No further changes are needed from my side.

🐇 ✅


✏️ Learnings added
Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/439

Timestamp: 2026-07-17T04:38:55.969Z
Learning: In `src/support/src/Number.php`, `Number::pairs()` intentionally supports negative step values by normalizing their magnitude with `abs()`, matching Laravel behavior and Hypervel’s compatibility contract; reviews should not request rejecting negative steps.

Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/439

Timestamp: 2026-07-17T04:38:55.969Z
Learning: When Hypervel atomically replaces a PHP/bootstrap file during a read-modify-write operation, both OPcache invalidations are intentional: the first protects the read sequence and the second invalidates the newly replaced file for subsequent loads.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown

Thanks for the detailed follow-up. The 52b47e07a changes look correct:

  • The ReflectionNamedType guard in DataObject::getDependencyFromUnionType is clean, and the DNF regression covers the exact previously-fatal path.
  • Js::encode() now applies UTF-8 substitution consistently across both the Jsonable and direct encoding branches.
  • string $key on the previous-key callback tightens the boundary without changing behavior.
  • The scratch-directory isolation in the provider-file tests removes the parallel-worker collision risk.

The rationales for the items left unchanged are all sound:

  • Number::pairs() negative step: confirmed — abs($by) is the established Laravel contract, both suites test it explicitly, and the zero guard already covers the degenerate case. Withdrawing that suggestion.
  • Both opcache_invalidate calls: the pre-read call guards the require in the read-modify-write sequence; the post-write call covers future loads of the replaced file. Two separate boundaries, both intentional.
  • #[RequiresPhpExtension('intl')] on testCounted: ext-intl is a root-level requirement and the suite cannot run without it. No attribute needed.

Everything looks good to me. The PR is in solid shape.

@binaryfire
binaryfire merged commit 34ce4d6 into 0.4 Jul 17, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant