Repository navigation
[CTX-1004] fix(lua,runtime): tighten bitty.env key bound to ADR 0006 (#1751) - #1752
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe host bridge now accepts environment keys that match ChangesEnvironment key contract
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The bridge intentionally rejects lowercase and over-64-byte keys under the updated contract, and the supplied implementation and test summaries agree. No actionable merge-blocking mismatch is evident; the change is mergeable subject to normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The stricter validation preserves grant-gated environment access. Existing integrations may need updated keys or grants; compatibility with deployed configurations has not been confirmed. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Xuepoo
left a comment
There was a problem hiding this comment.
Independent review (reviewer: pr-review-20261007-reviewer). Verdict: NEEDS-FIX — CI is red; code itself verifies clean.
Code verification (PR head 0b440a2):
- ENV_KEY_MAX_BYTES 128 -> 64 (host.rs:193). Shape is uppercase-only: first byte is_ascii_uppercase|'' with all bytes uppercase|digit|'', length 1..=64, empty rejected — equivalent to ^[A-Z_][A-Z0-9_]*$. Length uses byte count, correct for the ASCII-only shape.
- All validate_env_key failure modes (empty / over-bound / bad-first / bad-rest) now emit E_ENV_KEY_INVALID; zero env-key-related E_DEF_* remnants in the PR head. Trait docs and grant-extractor docs updated to the ADR bound.
- env_grant_shape_ok delegates to env_key_shape_ok, so grants tighten consistently (lowercase can never grant); env_grant_authorizes gates keys on the same predicate. Matcher logic otherwise unchanged.
- Tests pin the contract: accept HOME, _X1, A_B9, 64xA; reject "", 9LIVES, "has space", lower-ok?, home, Home, _x1, 65xA — all E_ENV_KEY_INVALID. Non-string arg stays E_DEF_INVALID (correct: argument-type error, not key shape). Grant tests pin lowercase rejection.
- Scope is 4 files, all env-key paths (host.rs, lua_parity.rs, plugin_runtime/mod.rs docs, services.rs test). No grant-gating, audit, or redaction behavior change. Hygiene OK: Closes #1751, Task CTX-1004, labels/milestone set, branch ctx-1004/fix-env-key-adr0006.
Blocker (not caused by this diff, still merge-blocking):
- Quality gates (2) FAIL: 2784/2785 pass, 1 failure in bitty-runtime::plugin_debug trace_handles_are_isolated_between_plugins (tests/plugin_debug.rs:314, xuepoo.tracer:start Lua error) — unrelated module, no env path. Other jobs (Linux/Windows/macOS, Quality gates 1) still pending; MSRV/Supply-chain/VM-tier pass. CodeRabbit review still in progress with no findings posted yet. mergeStateStatus BLOCKED, REVIEW_REQUIRED.
Action: re-run/investigate the plugin_debug failure, wait for full CI green plus CodeRabbit completion. Do not merge while red. I will re-review on a green head.
Priority: P1 | Area: area:lua,area:security,area:runtime | Labels: bug,P1,area:lua,area:security,area:runtime | Milestone: v0.1.0 | RFC: OQ-031 | Task: CTX-1004
Closes #1751.
Implements #1751 per bitty-docs#381 Option 1 (tighten host; ADR 0006 stays normative; see bitty-docs PR #444).
Change
crates/bitty-lua/src/host.rs:ENV_KEY_MAX_BYTES128 -> 64;env_key_shape_okto^[A-Z_][A-Z0-9_]*$1..64 (uppercase + digits only);validate_env_keyemitsE_ENV_KEY_INVALID(validation class) for empty / over-bound / shape failures before any grant check; updated bridge docs + unit tests (max-length accept, lowercase/mixed-case reject, grant shape/authorizes pins).crates/bitty-lua/tests/lua_parity.rs:env_bridge_rejects_malformed_keys_before_grantsnow expectsE_ENV_KEY_INVALIDfor empty/shape/mixed-case/65-byte keys; non-string argument staysE_DEF_INVALID.crates/bitty-runtime/src/plugin_runtime/services.rs:env_key_shape_rejected_before_grantsexpectsE_ENV_KEY_INVALID, adds lowercase/mixed-case cases.crates/bitty-runtime/src/plugin_runtime/mod.rs:env_grant_keysdoc now states the ADR 0006^[A-Z_][A-Z0-9_]*$prefix bound.specifications/unsafe-ffi-audit.md: no env-key rows exist (onlystd::env::set_vartest row); no update needed.Acceptance
home,Home,_x1) and over-64-byte keys are rejected withE_ENV_KEY_INVALIDbefore any grant check.E_NOT_IMPLEMENTEDdesensitized denial), audit, and redaction behavior unchanged.Evidence
cargo fmt --all -- --checkPASS.cargo clippy --workspace --all-targets -- -D warningsPASS.cargo check --target x86_64-pc-windows-gnu -p bitty-lua -p bitty-runtime --all-targetsPASS (no platform-conditional code touched; cross-check anyway).cargo test -p bitty-lua --libPASS (94 passed).cargo test -p bitty-lua --test lua_parityPASS (17 passed).cargo test -p bitty-lua --test host_bridgePASS (39 passed).cargo test -p bitty-runtime --lib plugin_runtimePASS (222 passed).Do NOT merge: leaving open for review.
Summary by CodeRabbit