Skip to content

AUDIT.md

Latest commit

 

History

History
274 lines (221 loc) · 19 KB

AUDIT.md

File metadata and controls

274 lines (221 loc) · 19 KB
Raw

phprs Codebase Audit

Scope: Rust PHP interpreter workspace (/Users/yingkitw/Desktop/myproject/phprs)
Audited against: SPEC.md, ARCHITECTURE.md, README.md, TODO.md, AGENTS.md, plus automated checks (cargo build, cargo test --workspace, cargo clippy --workspace --all-targets, cargo fmt -- --check).
Date: 2026-07-25
Commit: 5ede6a3 on main (with two uncommitted worktree changes).
Note: Counts and statuses below are as of the audit date; see TODO.md and README.md for current numbers.

Update after fixes: The critical/high issues below have been addressed. cargo build --workspace, cargo test --workspace, and cargo fmt -- --check now pass; cargo clippy --workspace --all-targets reports no errors and ~50 style warnings (down from ~210).

Executive Summary

The project is now in a green build/test/format state. All workspace tests pass, formatting is clean, and cargo clippy no longer errors. Documentation drift has been corrected. A number of style warnings remain in clippy, but they do not block correctness.

Check Result
cargo build --workspace ✅ Passes
cargo test --workspace ✅ Passes (485+ at audit date; 510+ as of 2026-08-24)
cargo clippy --workspace --all-targets ✅ No errors; ~50 style warnings remain
cargo fmt -- --check ✅ Clean
Documentation sync ✅ Exception/opcode drift fixed

Most critical issues (now fixed):

  1. The failing function_result_array_access test has been fixed by applying access-chain parsing after function-call and parenthesized expressions in src/engine/compile/expression/operators.rs.
  2. The dead duplicate src/engine/vm/handlers.rs module has been removed.
  3. TODO.md and ARCHITECTURE.md now correctly reflect implemented exception handling and 73 opcodes.

Overall quality score: 8/10 — strong test coverage and now clean CI basics; remaining work is to drive down clippy style warnings and resolve the medium/low design notes below.

Fixes Applied

  • Fixed func()['key'] / (func())['key'] subscripting in the expression parser.
  • Removed 1,390-line dead src/engine/vm/handlers.rs duplicate.
  • Fixed all cargo clippy errors (approx_constant literals, absurd_extreme_comparisons, missing safety docs).
  • Ran cargo fmt across the workspace; added formatting check.
  • Updated TODO.md to mark exception dispatch and func-result array access as completed and removed stale gap notes.
  • Updated ARCHITECTURE.md opcode count from 67 to 73.
  • Updated README.md to accurately describe the current CI state.
  • Removed pub use engine::vm; re-export; standardized all internal imports on crate::engine::vm.
  • Fixed bin/phprs CLI version to derive from CARGO_PKG_VERSION.
  • Removed stray .DS_Store files.
  • Fixed php_is_writable open-options intent and removed unused imports/dead helpers flagged by clippy.

Detailed Findings

Critical

1. cargo test --workspace fails on a known, documented VM bug

  • Location: src/engine/vm/tests.rs:361-367
  • Severity: Critical
  • Category: Correctness / CI
  • Description: The test test_compile_and_execute_function_result_array_access expects values()["x"] and (values())["x"] to return the indexed element. Instead the VM returns the whole array, so output is "Array" rather than "hellohello". This is the exact gap documented in TODO.md (“func()['key'] / (func())['key'] — subscripting a function-call result directly returns the whole array instead of the element”), but the test was added without a corresponding fix.
  • Impact: Every full test run fails. The failure masks the otherwise-passing integration suite and violates the SPEC.md requirement that cargo test --workspace must succeed.
  • Recommendation: Either (a) fix FetchDim/primary.rs to correctly dereference a function-call result before indexing, or (b) mark the test #[ignore = "tracked bug: func()['key'] indexing"] until it is fixed. The current state of leaving a failing regression test in tree is worse than either option.
  • Suggested command: cargo test -p phprs --lib engine::vm::tests::test_compile_and_execute_function_result_array_access -- --nocapture

2. src/engine/vm/handlers.rs is unused dead code

  • Location: src/engine/vm/handlers.rs (1,390 lines)
  • Severity: Critical
  • Category: Maintainability / Leanness
  • Description: The module is declared as mod handlers; in src/engine/vm/mod.rs but no code path imports from it. The active dispatch table is built from src/engine/vm/dispatch_handlers.rs (2,430 lines). The old file contains a near-duplicate set of opcode handlers, many of which are stubbed or marked #[allow(dead_code)].
  • Impact: It bloats compile times, confuses navigation, and creates a risk that future edits are applied to the wrong copy. It is the source of the stale “exception handlers are dead code” note in TODO.md.
  • Recommendation: Delete src/engine/vm/handlers.rs and remove mod handlers; from src/engine/vm/mod.rs. If any logic in the old file is still valuable, port it to dispatch_handlers.rs first.
  • Suggested command: git rm src/engine/vm/handlers.rs

3. cargo clippy --workspace --all-targets does not pass

  • Location: Multiple source/test files
  • Severity: Critical
  • Category: Tooling / Quality bar
  • Description: Clippy reports 12 errors and ~210 warnings. The errors are:
    • 11 × approx_constant in tests that use the literal 3.14 instead of std::f64::consts::PI.
    • 1 × absurd_extreme_comparisons in src/php/hash.rs:509 (if length <= 0 where length is usize).
  • Impact: A project that cites clippy as a development tool cannot merge code that fails it. These are trivial fixes, but their presence shows the quality gate is not enforced.
  • Recommendation: Fix the 3.14 literals in tests, change length <= 0 to length == 0, then run cargo clippy --workspace --all-targets -- -D warnings until clean. Consider adding a CI job that fails on clippy warnings.

4. cargo fmt -- --check reports formatting drift

  • Location: bin/phprs/src/main.rs, bin/phprs/src/pkg/commands/init.rs, bin/phprs/src/pkg/commands/install.rs
  • Severity: Critical
  • Category: Tooling / Consistency
  • Description: Several CLI files do not match rustfmt style. The diff includes import ordering, multi-line method chains, and match arm formatting.
  • Impact: Formatting drift makes reviews noisy and contradicts the README claim that “the workspace runs clean (no warnings) on cargo test --workspace and cargo build --workspace” (format is part of the implied quality bar).
  • Recommendation: Run cargo fmt and commit the result. Add a CI formatting check.

High

5. Documentation drift: exception handling

  • Location: TODO.md:167-168
  • Severity: High
  • Category: Documentation accuracy
  • Description: TODO.md still states: “Exceptions are not wired into the VM dispatch. throw, try/catch/finally opcodes … are no-ops in the real dispatch table.” The latest commit (5ede6a3) is titled “Implement try-catch exception handling in the PHP interpreter,” and the dispatch table in src/engine/vm/execute.rs wires TryCatchBegin, TryCatchEnd, CatchBegin, CatchEnd, FinallyBegin, FinallyEnd, and Throw to real handlers in dispatch_handlers.rs.
  • Impact: Contributors will make incorrect assumptions about what works. README.md and SPEC.md list try/catch/finally as supported, while TODO.md claims the opposite.
  • Recommendation: Update TODO.md to move exception handling to Completed, remove the stale warning, and note the remaining limitation (cross-function propagation is not yet supported). exception_dispatch.rs already documents this limitation accurately.

6. Documentation drift: opcode count inconsistency

  • Location: ARCHITECTURE.md:61 (67 opcodes) vs. README.md:498 / TODO.md:37 (73 opcodes)
  • Severity: High
  • Category: Documentation accuracy
  • Description: The architecture doc says 67 opcodes; the README and TODO say 73. The enum in src/engine/vm/opcodes.rs defines 73 variants (0–72).
  • Impact: A trivial but visible inconsistency that undermines confidence in the rest of the docs.
  • Recommendation: Update ARCHITECTURE.md to 73 opcodes.

7. Documentation drift: README claims clean workspace tests

  • Location: README.md:474
  • Severity: High
  • Category: Documentation accuracy
  • Description: The README says: “The workspace runs clean (no warnings) on cargo test --workspace and cargo build --workspace.” This is currently false because of the failing test, clippy errors, and formatting drift.
  • Impact: Misleading new contributors and users.
  • Recommendation: Restore the green state first, then keep the claim. Until then, rephrase to “cargo build --workspace passes; cargo test has one tracked regression; clippy/fmt drift is being addressed.”

8. Inconsistent import path: crate::vm vs. crate::engine::vm

  • Location: src/engine/jit.rs, src/engine/function_optimizer.rs, src/engine/opcode_cache.rs, src/engine/benchmark.rs, src/engine/array_ops.rs
  • Severity: High
  • Category: Maintainability / Consistency
  • Description: These files import via crate::vm::*. The root lib.rs makes this work with pub use engine::vm;, but the rest of the crate (and new contributors) will naturally expect crate::engine::vm. Having two valid paths to the same module is confusing.
  • Impact: Increases cognitive load and makes refactor searches harder.
  • Recommendation: Standardize on crate::engine::vm everywhere and remove the pub use engine::vm; re-export. If external consumers rely on phprs::vm, keep the re-export but document it; otherwise drop it.

9. unsafe impl Send / unsafe impl Sync for PhpValue and PhpObject

  • Location: src/engine/types.rs:181-182, src/engine/types.rs:432-433
  • Severity: High
  • Category: Soundness / Concurrency
  • Description: The comments explicitly state that the traits are implemented manually and “actual thread safety must be ensured by the caller.” These types contain raw pointers (*mut via PhpString, PhpArray, PhpObject), reference-counted cells, and mutex-free GC state.
  • Impact: If PhpValue is ever shared across threads without external synchronization, the program has undefined behavior. This contradicts the marketing claim of “fearless concurrency” for the host code.
  • Recommendation: Either prove the types are genuinely Send/Sync and document the invariants, or remove the implementations and force callers to use Arc<Mutex<…>> or channel-based designs. The current “caller must ensure” handoff is not sufficient for a soundness claim.

Medium

10. Uncommitted work in progress on the failing test and parser

  • Location: src/engine/vm/tests.rs and src/engine/compile/expression/primary.rs
  • Severity: Medium
  • Category: Repository hygiene
  • Description: git status shows two modified files with no commit. The test file adds the failing function_result_array_access test; the parser file has a 6-line change attempting to fix it.
  • Impact: It is unclear whether the fix is expected to land with the test or whether this is accidental WIP on main.
  • Recommendation: Finish the fix in a branch, ensure tests pass, and commit. Do not leave partially-fixed bugs on the default branch.

11. random_int/random_bytes has subtle correctness issues

  • Location: src/php/hash.rs:502-547
  • Severity: Medium
  • Category: Correctness / Security
  • Description:
    • length <= 0 is a clippy error and is logically redundant for usize.
    • i64::from_ne_bytes(bytes).abs() can panic in debug on i64::MIN and does not improve uniformity.
    • Modulo reduction random_val as u64 % range introduces measurable bias for non-power-of-two ranges.
  • Impact: A biased or panicking random-int implementation is a problem for a function used for tokens and security-sensitive values.
  • Recommendation: Use rand::Rng::gen_range or the getrandom crate with rejection sampling. At minimum, remove the .abs() and handle i64::MIN; better, generate a u64 directly and reject values above u64::MAX - (u64::MAX % range).

12. Approximate PI literals in math tests

  • Location: src/php/math.rs, src/engine/types/tests.rs, src/engine/vm/builtin_capability_tests.rs
  • Severity: Medium
  • Category: Test quality
  • Description: Literal 3.14 and 3.14159 trigger clippy::approx_constant. They also make the tests slightly less precise than intended.
  • Impact: Blocks clippy and gives the impression that tests were written without attention to floating-point constants.
  • Recommendation: Replace with std::f64::consts::PI and format precision as needed.

13. file_get_contents HTTP support makes external requests

  • Location: src/php/http_stream.rs
  • Severity: Medium
  • Category: Security / Sandboxing
  • Description: file_get_contents('http://...') and file_get_contents('https://...') are supported. There is no evidence of allowlists, timeouts, request size limits, or sandboxing in the host.
  • Impact: Running untrusted PHP code could lead to SSRF, unexpected egress traffic, or resource exhaustion.
  • Recommendation: Document the behavior and add runtime limits: request timeout, max response size, and optionally an allowlist of URL schemes/hosts. Consider a --no-network CLI flag.

14. file_put_contents may not truncate existing files

  • Location: src/php/filesystem.rs:164
  • Severity: Medium
  • Category: Correctness
  • Description: Clippy warns that the file is opened with .create(true) but without .truncate(true) or .append(true). The intent is unclear from the code.
  • Impact: file_put_contents may overwrite only the prefix of an existing file, leaving stale trailing bytes — a PHP semantic mismatch.
  • Recommendation: Add .truncate(true) if mimicking PHP file_put_contents, or .append(true) if append mode is intended. Add a regression test.

15. Large number of clippy style warnings

  • Location: Across the workspace (collapsible if, needless ?, manual strip, get(0) instead of first, redundant closures, etc.)
  • Severity: Medium
  • Category: Code style / Maintainability
  • Description: ~210 warnings, many duplicated across test modules. While not errors, they indicate the code was not run through clippy before commit.
  • Impact: Noise hides real issues; reviews are slower.
  • Recommendation: After fixing the 12 errors, run clippy and address warnings category-by-category (or suppress intentionally unhelpful lints in Cargo.toml / clippy.toml).

16. TODO.md still lists “in progress” items that appear completed

  • Location: TODO.md Planned/Standard Library section
  • Severity: Medium
  • Category: Documentation accuracy
  • Description: Many standard-library items are checked [x] under “Planned” rather than moved to “Completed.” The checkboxes are also used under both sections, which makes progress hard to read.
  • Impact: Makes backlog triage difficult.
  • Recommendation: Reorganize TODO.md so completed work lives under Completed and only unfinished work remains under Planned. Align with AGENTS.md step 1.

Low

17. .DS_Store file present in working tree

  • Location: /Users/yingkitw/Desktop/myproject/phprs/.DS_Store
  • Severity: Low
  • Category: Repository hygiene
  • Description: macOS metadata file exists at repo root. It is already covered by .gitignore, so it is not tracked, but it is visible in the working tree.
  • Impact: Minor clutter.
  • Recommendation: rm .DS_Store && git status.

18. README feature table shows WordPress as “✅” in one place and “🚧” in another

  • Location: README.md:57 vs. README.md:297-301
  • Severity: Low
  • Category: Documentation consistency
  • Description: The feature grid marks WordPress as under construction, while the later section says it is “partial / examples/ focus.” These are not contradictory but inconsistent in tone.
  • Impact: Minor confusion.
  • Recommendation: Use one status symbol consistently.

19. Version badge on CLI says 0.1.0, library is 0.1.14

  • Location: bin/phprs/src/main.rs:16
  • Severity: Low
  • Category: Consistency
  • Description: #[command(version = "0.1.0")] while Cargo.toml has version = "0.1.14".
  • Impact: phprs --version will report the wrong version.
  • Recommendation: Derive the version from env!("CARGO_PKG_VERSION") or update the literal to 0.1.14.

Positive Findings

  • Broad test coverage: 485+ tests at audit date (510+ as of 2026-08-24), including 15 integration tests for root examples/*.php and framework stubs. The integration suite (tests/examples_runtime.rs) is a strong pattern and should be maintained.
  • Honest performance claims: PERFORMANCE.md and the README avoid fabricated speedup numbers and clearly state that PHP comparisons require reproducible benchmarks.
  • Documentation-first culture: SPEC.md, ARCHITECTURE.md, TODO.md, and AGENTS.md exist and are detailed. With the drift items fixed, they form a solid foundation.
  • Incremental compatibility strategy: The project correctly scopes framework support as demos and warns against treating them as production parity.
  • String interning: src/engine/string_intern.rs is a clean, tested, and well-documented utility.

Recommendations by Priority

Immediate (this session)

  1. Fix or ignore the function_result_array_access failing test.
  2. Run cargo fmt and commit.
  3. Fix the 12 clippy errors (replace 3.14 literals with PI, fix length <= 0).
  4. Delete src/engine/vm/handlers.rs (dead duplicate code).
  5. Update TODO.md exception note and ARCHITECTURE.md opcode count.

Short-term (next sprint)

  1. Add a CI workflow that runs cargo fmt -- --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --no-fail-fast.
  2. Standardize imports on crate::engine::vm and remove the vm re-export.
  3. Review unsafe impl Send/Sync for PhpValue / PhpObject and either justify or remove them.
  4. Fix random_int bias and potential abs() overflow.

Medium-term

  1. Resolve the remaining clippy warnings systematically.
  2. Add SSRF/network limits to file_get_contents HTTP support.
  3. Reorganize TODO.md so completed items are clearly separated from backlog.

Verification Commands

# Build
cargo build --workspace

# Test (will fail until the array-access test is addressed)
cargo test --workspace --no-fail-fast

# Lint (will fail until clippy errors are fixed)
cargo clippy --workspace --all-targets -- -D warnings

# Format check
cargo fmt -- --check

# Inspect the failing test specifically
cargo test -p phprs --lib engine::vm::tests::test_compile_and_execute_function_result_array_access -- --nocapture