test(app-lock): cover node classification and the kind → mode default - #500
Conversation
|
@codex review Generated by Claude Code |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
`kind` and `mode` are the two lockfile fields an approver reads: the Glass Box colours a card red and demands a `safety:` block on `mode: write`, grey and silent on `mode: read`. The renderer was tested against hand-built `CompiledNode`s, but nothing tested that `compile` puts the right two strings into the lock in the first place — `classify_node` had no test at all, and coverage put six of its ten arms, plus the whole agent-not-installed branch of `compile_node`, at zero. Eight tests, each proven red under a mutation of the code it covers: - every primitive is named by its own kind in the lock (all ten arms) - only the observing primitives default to read mode; an agent node still takes its mode from the manifest - a node declaring no primitive is `unknown` and falls to write - a `do:` body node is classified by its own declaration, not its parent's - an uninstalled agent warns whatever mode the author declared, and carries no output schema - a command missing from an installed manifest informs when told and warns when guessing — the branches are indistinguishable by mode alone - a reference from an assert expression or a sweep value still orders the node after its source - a reference buried in a list value is ref-checked like any other
fe3f800 to
2d8933a
Compare
|
Force-pushed to re-author the single commit under New head is @codex review Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Area picked:
cli/src/app_lock.rs— what the lockfile tells an approverkindandmodeare the twoCompiledNodefields a human actually reads.render_glass_box_html(cli/src/commands/app.rs:1786) colours a card red andstamps
safety MISSINGonmode: write, grey and silent onmode: read, andlabels an agent-less node by its
kind. That renderer has nine tests — all againsthand-built
CompiledNodefixtures. Nothing tested thatcompileputs the right twostrings into the lock in the first place.
cargo llvm-cov --all-targetsonmainput the whole ofclassify_node's tail atzero (
assert,compare,approve,snapshot,model-lock,unknown), togetherwith
compile_node's entire agent-not-installed branch, its declared-write arm for amissing command,
collect_refs' sequence arm, andderive_connections' assert/sweepscanners. So a primitive that drives a live-model write could have compiled to
mode: readand shown up grey, with no missing-safety badge, on the one surface arun is approved from — and every test in the repo would still have been green.
Eight tests added, all in the existing
app_lock::testsmodule. No tests deleted —see the last section.
Mutation evidence
Every test below was proven red by breaking the code it covers and green again on
restore. Each mutation killed exactly its target test and no other new test, so
none of the eight is riding on another's assertions.
src/app_lock.rsclassify_node: thesweeparm answers"for-each"every_declared_primitive_is_named_by_its_own_kind_in_the_lockclassify_node: the fallback answers"inline"instead of"unknown"…named_by_its_own_kind…anda_node_that_declares_no_primitive_is_unknown_and_falls_to_writecompile_node: read-mode default widened with|| kind == "for-each"only_the_observing_primitives_default_to_read_modecompile_node:kind == "snapshot"dropped from the read-mode defaultonly_the_observing_primitives_default_to_read_modecompile_node: agent branch returns a hardcoded"write"instead ofresolved.modeonly_the_observing_primitives_default_to_read_modecompile: body nodes are stamped with their top-level ancestor's kinda_do_body_node_is_classified_by_its_own_declaration_not_its_parentsmode: readnote demotedwarn→infoan_uninstalled_agent_is_warned_about_whatever_mode_the_author_declaredmode: writeresolves to"read"an_uninstalled_agent_is_warned_about_whatever_mode_the_author_declaredmode: writenote promotedinfo→warna_command_missing_from_an_installed_manifest_informs_when_told_and_warns_when_guessingderive_connections: theassert.exprscan deleteda_reference_from_an_assert_or_a_sweep_still_orders_the_node_after_its_sourcederive_connections: thesweep.valuesscan deleteda_reference_from_an_assert_or_a_sweep_still_orders_the_node_after_its_sourcecollect_refs: theValue::Sequencearm removeda_reference_buried_in_a_list_value_is_ref_checked_like_any_otherNotes on the two mutations that are worth reading twice:
only_the_observing_primitives_default_to_read_modemore than akind-table restatement.
agentsits on the write side of that table, yet aread-mode command must still compile to
read— the test asserts both an agent nodewith a
mode: readcommand and one with amode: writecommand, so the two rulesstay distinguishable on nodes where they would otherwise agree.
…informs_when_told_and_warns_when_guessing:both branches compile to
mode: write, so severity is the only thing that separates"the author already made this call" (info) from "the compiler had to guess" (warn),
per Compile notes are untyped strings — add a severity/kind so consumers can render info vs warning without parsing prose #170. The test asserts the two kinds and then asserts they differ.
Vacuity check on the new tests
Against the three failure modes called out for Rust:
field off a
LockFilereturned bycompile, or an edge list returned byderive_connections. The fixtures are.flosource text and agent manifests; theassertions are on compiler output.
unwrap()standing in for an assertion. Thecompiled()helper panics withthe missing node's id when a node is absent from the lock — that is a real failure
(M6 reaches it), not an accidental one, and it is never the only check in a test.
No deletions this run
I looked for tests in this area that assert nothing meaningful and did not find any.
The candidates I checked and why each survives:
glass_box_testsincli/src/commands/app.rsbuildCompiledNodefixturesby hand, which reads like the "asserts on its own fixture" pattern — but they are
testing a renderer, and the fixture is its input. Breaking the escaper or the
mode→class mapping fails them. They earn their place; this PR supplies the upstream
half they were missing.
app_lock::testsall drivecompile/derive_connectionsandassert on output. None is vacuous.
One observation for the maintainer, deliberately not acted on here: the four
extract_group_*tests incli/src/commands/tree.rs:186-217are byte-identical to thefour in
cli/src/commands/mod.rs:59-90and, viause super::*, exercise the samecrate::commands::extract_group. They are pure duplicates. I left them alone for tworeasons — breaking
extract_groupkills both copies, so the redundancy cannot bedemonstrated by the mutation discipline this routine deletes tests under, and #499
(
routine/abstractions-2026-09-07) is open over collapsing re-typed abstractions,which is where that duplication properly belongs.
Gates
Prepared as CI does:
clang libsecret-1-dev libdbus-1-dev pkg-configvia apt, toolchainpinned from
cli/rust-toolchain.toml(1.95.0). Run fromcli/:cargo fmt --all -- --check— cleancargo clippy --all-targets -- -D warnings— cleancargo test— 1620 passed, 0 failed🤖 Generated with Claude Code
https://claude.ai/code/session_016AhfthzLr7vdb8iuJtRdsL
Generated by Claude Code