Repository navigation
Prove the probe shim-proof behaviourally, and make profile-mark collisions unrepresentable - #189
Conversation
…rder The shim-proofness property -- any invocation of the candidate claude triggers the very download the probe exists to detect -- was asserted by scrubbing the rendered script's text for an invocation. The re-verdict on #164 showed that guard walking straight past a probe that executed "$(command -v claude)": after the lookup is scrubbed, no literal 'claude' remains in the execution, and the suite stayed green under the mutation. The guard is now behavioural: the probe runs for real against a claude that records being invoked, and the record must stay empty. The recorder is shell builtins only (':' and a redirection), because the probe runs on a stripped PATH carrying nothing but readlink -- a marker needing an external binary would fail to record the very invocation it exists to catch, the constraint the reviewer flagged. The test also pins the answer (lendable), so a probe that crashed before resolving the claude cannot pass with a trivially empty record. Proven by mutation, selection stated: a probe line executing "$(command -v claude)" -- the reviewer's exact mutation -- turns the behavioural test red while the text scrub stays green. The scrub is kept as a complement (it confines the name to known lookups, catching an invocation on a branch one run does not take) under a docstring that now says what it actually guards, and no longer claims to be the property. Part of #166.
Every PATH line devlaunch appends to a container's login profile goes in under a '# devlaunch: <tag>' mark, and each guard is an exact-line match on that mark -- but nothing ensured the tags were distinct. Two lines under one mark silently drop whichever is appended second: its guard finds the first line's mark, reads it as its own work already done, and every script involved still exits 0. The re-verdict on #164 ran exactly that mutation and the suite stayed green. The tag is now derived from the line's content (twelve hex characters of its SHA-256), so two different lines cannot share a mark by construction -- derivation over assertion, as the ticket prefers. Identical lines sharing one mark is not a collision but the dedupe the guard exists for. Existing profiles marked by an older devlaunch gain one duplicate PATH entry the first time they are seen, the cost the module already accepts for any mark change, and nothing after. The property is held by a behavioural test: every append the provision and transfer scripts render runs for real, twice, into one profile -- one profile because that is where the lines meet over a workspace's life -- and each PATH line must land exactly once. Proven by mutation twice, selections stated: the reviewer's collision (transfer's tag set equal to provision's) turned it red under the old hand-picked tags, and forcing the derivation to a constant turns it red now; in both runs every other test stayed green. Part of #166.
.devcontainer/claude-code/install.sh was excluded from the #164 guard sweep on the stated grounds that it guards its own line in its own file. That reason was wrong: $TARGET_HOME/.profile is the user's login profile -- the same foreign file the sweep was about -- and its guards substring- matched '.pixi/bin' and 'pixi/envs/claude-shim', the very two strings provision_script had just stopped matching. Latent rather than live (the stock base-image profile contains neither string), but an exclusion resting on a wrong reason is how this class recurs, so the record is corrected here and the coupling removed. Its two profile edits are now devlaunch's _profile_prepend output pasted verbatim -- same content-hash mark, same exact-line fixed-string guard, same line. Verbatim rather than merely same-shaped, because the marks buy more than guard hygiene: the installer runs at image build and the provision script at up, and only matching marks let each recognise the other's work instead of appending its own copy of the same line. sh cannot rederive a content hash, so a test pins the fragments byte-for-byte to the renderer and names it as the regeneration path. The guard property tests extend their parametrization to the installer -- red before this change, which is the mutation that proves them: the old substring guards fail both the owned-mark and the whole-line property. The cross-script landing test now runs the installer's appends into the same profile as the other two scripts' and still requires every PATH line exactly once, which is the convergence the matching marks buy. The create case loses nothing: '>>' creates the file the old '>' branch created, and the chown that came with it survives unconditionally -- chowning the user's own profile to the user is a no-op everywhere else. Part of #166.
test_the_profile_is_not_appended_to_twice and test_the_profile_edit_is_guarded_like_the_network_installs both closed with 'every profile write starts with grep -q' -- the shape of the guard before #164 replaced it with an exact-line fixed-string match on a devlaunch-owned mark. 'grep -q' is a prefix of 'grep -qxF', so they kept passing, guarding nothing the stronger property tests do not: the whole-line guard shape is pinned by test_a_guard_matches_a_whole_line_not_a_fragment_of_one, the every-edit- guarded property by test_every_line_lands_exactly_once_however_often_the _scripts_run, and each write's existence by the extraction both share. With the vestigial assertion gone, all either test still said was 'the scripts append to the profile', which that shared extraction already refuses to leave vacuous -- so the methods go whole rather than staying as empty names. Part of #166.
Reviewer's GuideThis PR strengthens PATH/profile guards and shim-proofness for the Claude probe, derives devlaunch profile marks from content hashes, and makes the devcontainer feature installer share the same guarded, idempotent profile edits and behavioural tests as the main scripts. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
len(appends) == 5assertion intest_every_line_lands_exactly_once_however_often_the_scripts_runhard-codes current PATH edit count and will become brittle as scripts evolve; consider deriving expectations from the scripts themselves rather than a fixed number. - The hard-coded
# devlaunch: <hash>marks in.devcontainer/claude-code/install.shduplicate the hashing logic in_profile_prependand risk drifting if the line content changes; it may be safer to generate these marks from the Python helper as part of build or templating rather than inlined literals. - The
FEATURE_INSTALLERpath resolution viaparents[2] / '.devcontainer/claude-code/install.sh'assumes a specific test file layout; consider making this path derivation more robust (e.g. using a project root helper or configuration) to avoid breakage if the test directory structure changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `len(appends) == 5` assertion in `test_every_line_lands_exactly_once_however_often_the_scripts_run` hard-codes current PATH edit count and will become brittle as scripts evolve; consider deriving expectations from the scripts themselves rather than a fixed number.
- The hard-coded `# devlaunch: <hash>` marks in `.devcontainer/claude-code/install.sh` duplicate the hashing logic in `_profile_prepend` and risk drifting if the line content changes; it may be safer to generate these marks from the Python helper as part of build or templating rather than inlined literals.
- The `FEATURE_INSTALLER` path resolution via `parents[2] / '.devcontainer/claude-code/install.sh'` assumes a specific test file layout; consider making this path derivation more robust (e.g. using a project root helper or configuration) to avoid breakage if the test directory structure changes.
## Individual Comments
### Comment 1
<location path="CHANGELOG.md" line_range="12" />
<code_context>
### Fixed
+- **Two guards around the tools lend were weaker than what was claimed of them**
+ ([#166](https://github.com/blooop/devlaunch/issues/166)) — the follow-ups the
+ final re-verdict on [#164](https://github.com/blooop/devlaunch/pull/164) judged
</code_context>
<issue_to_address>
**suggestion (typo):** The phrase "tools lend" is unclear and may be a typo or miswording.
In the heading, the phrase “tools lend” is difficult to understand and may be a typo or incorrect wording. Please rephrase (e.g., clarify whether it should be possessive or use a different noun) so the heading’s meaning is immediately clear.
```suggestion
- **Two guards around the tools were weaker than what was claimed of them**
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| ### Fixed | ||
|
|
||
| - **Two guards around the tools lend were weaker than what was claimed of them** |
There was a problem hiding this comment.
suggestion (typo): The phrase "tools lend" is unclear and may be a typo or miswording.
In the heading, the phrase “tools lend” is difficult to understand and may be a typo or incorrect wording. Please rephrase (e.g., clarify whether it should be possessive or use a different noun) so the heading’s meaning is immediately clear.
| - **Two guards around the tools lend were weaker than what was claimed of them** | |
| - **Two guards around the tools were weaker than what was claimed of them** |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #189 +/- ##
=======================================
Coverage 95.33% 95.34%
=======================================
Files 22 22
Lines 3025 3026 +1
=======================================
+ Hits 2884 2885 +1
Misses 141 141
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed at merge-base dcdb077 (three-dot diff), spec = #166. Checks green (12/12). Per the run's steer, every mutation claim was re-executed in an isolated worktree rather than trusted; results in the third section. The two axes below are independent and unmerged.
Standards
1. Low — dead/inconsistent filter arm in TestProfileGuards. _guards (test/unit/test_tools.py:200) matches both >> "$PROFILE" and >> "$profile", but after this diff no script writes lowercase (install.sh switched to PROFILE), so the lowercase arm is dead. Worse, the sibling collectors match only uppercase (test_tools.py:241, and pre-existing :141), so if a lowercase append ever reappeared it would pass the guard-shape tests while being silently excluded from the exactly-once run and the == 5 count — the exact "guard that lies" failure this PR exists to remove. Drop the arm or apply it in all three places.
2. Low — PROFILE breaks the function's local idiom. .devcontainer/claude-code/install.sh:100 declares PROFILE as a global inside install_claude_code, where every other variable is local. The rendered fragment forces the name, not the scope; local PROFILE=... keeps the file consistent.
3. Info — hardcoded hash marks: duplicated knowledge, adequately mitigated. install.sh:101-102 paste marks only devlaunch/tools.py:217 can derive. Verified: 6b593c3a6327 / 12897b113bea are the correct SHA-256[:12] of the guarded lines. The byte-for-byte pin (test_tools.py:267-289) plus regeneration instructions in both the installer's comment and the assert message make the duplication detectable and repairable; given sh can't rederive the mark, a defensible trade, well documented.
4. Info — assert len(appends) == 5 (test_tools.py:242) is a magic number, but an intentional tripwire with a failure message telling the maintainer what to do; the per-line count == 1 checks carry the real property.
Private tools._profile_prepend in tests matches existing repo idiom (test_tools.py:648 does the same with _claude_source). README unaffected — no CLI/flag change; its claims about install.sh and profile-sourcing remain accurate. CHANGELOG entry present and in house style. Nothing else from the Fowler baseline at flag-worthy severity.
Spec
Item 1 — delivered. "Make it behavioural: run the probe against an instrumented shim that records being called, replacing or joining the text scrub" — done at test/unit/test_tools.py:692: the probe runs for real, the shim records via : > "$HOME/shim-was-executed", and the record must stay empty; the scrub joins rather than stands alone (:716). Constraint honoured: "the instrumentation marker must not depend on a binary absent from the probe's stripped PATH" — the recorder is a : builtin plus redirection under an absolute-path #!/bin/sh shebang; _bare_path links in only readlink. The test pins LENDABLE, so a probe crashing pre-resolution can't pass with a trivially empty record. Seam matches the ticket's.
Item 2 — delivered. "Prefer deriving the tag so a collision is unrepresentable" — derivation chosen (devlaunch/tools.py:217, sha256(line)[:12]); the tag parameter is gone entirely, so the ticket's seam ("two scripts given the same tag") is unrepresentable in the new code.
Item 3 — delivered. "Convert it to the same devlaunch-owned-mark guard, and correct the record" — install.sh:101-102 now carries grep -qxF '# devlaunch: <hash>' guards; the record is corrected in comments, tests, commit e89d027, and CHANGELOG. "the same string-level property test already parametrized over provision_script/transfer_script, extended to this script" — both parametrized tests extend to "feature" (test_tools.py:184, 206, 259).
Item 4 — delivered, slightly beyond. The startswith("grep -q") assertions at old test_tools.py:142, 984 are gone; the diff deletes the whole enclosing methods, justified in commit dac0507 (the remainder was vacuous) — more than the ticket's literal ask, but not against it.
Closing requirement ("Every new or changed guard must be proven by mutation, with the selection stated") — selections stated per-item in the PR body and commits; re-execution below confirms all bite. Out-of-scope check: attach_workspace/setup_hostname — zero occurrences in the diff, correctly left for #168. Unrequested extras (byte-for-byte pin test, cross-script landing test, install.sh's create-branch folded into >> with unconditional chown) all support the four items; none contradict the spec.
Mutation claims, re-executed
Baseline at head ebec7d5: 80 passed.
Probe executes "$(command -v claude)" — CONFIRMED. Behavioural test red (shim-was-executed exists); the retained scrub stays green under the bare variant (1 failed, 1 passed) — it genuinely cannot see this execution, which is the PR's stated reason it no longer stands alone. (An unclaimed bonus: the scrub's --version tripwire caught a first variant that invoked claude --version.)
Tag collision under old code — CONFIRMED, with a caveat. Old suite + old code + collision: 77 passed — the old suite is blind, the original bug real (executing the collided appends drops transfer's .local/bin line entirely: count 0). Caveat: the new landing test is red in every mixed old/new state (it fails on the len(appends) == 5 harvest precondition, old install.sh's lowercase appends escaping it), so its red under this mutation is confounded rather than collision-specific.
Derivation forced to a constant — CONFIRMED in substance, exactness REFUTED. 2 failed, 78 passed: the landing test goes red as claimed, but the byte-for-byte pin test goes red too (constant mark vs install.sh's pasted hashes). The PR body's "each turns exactly the landing test red while every other test stays green" overstates isolation; both reds are correct detections.
Pre-change installer — CONFIRMED. 4 failed: the claimed owned-mark[feature] and whole-line[feature] reds occur; the landing and pin tests also fail (superset of the claim, same direction).
Steered probes of the derived mark:
- Truncation: 48 bits over ~3 lines (~10⁻¹⁴ collision odds); if one arrived anyway, the landing test catches it — it runs the real appends and a collision drops the later line's count to 0.
- Whitespace drift: a pasted line drifting by one space with its mark unchanged is caught by the pin test and stays deduped at runtime (guards match marks, not lines). Line-and-mark regenerated together after a drift lands the directory twice — only the pin test sees that, and a duplicate PATH dir is harmless, the cost the module already accepts.
- Regeneration drift: both pasted hashes verified correct; any future change to
_profile_prepend's rendering failsfragment in installer. Not caught by the pin: extra stale pasted fragments (intests presence, not absence) — but the landing test'slen(appends) == 5catches any extra append. - Real gap found (latent, non-blocking): profile-target mismatch. install.sh hardcodes
PROFILE="$TARGET_HOME/.profile"(:100) while provision/transfer resolve~/.bash_profile → ~/.bash_login → ~/.profile. On an image shipping~/.bash_profile, provision greps a different file than the installer wrote, never sees the mark, and appends again — the shared-mark recognition this PR advertises silently fails across files. Benign today (login shells read only one of the two, and a duplicate PATH dir is the accepted cost), and no test covers it. Same latent-not-live class as the ticket's item 3; deserves its own ticket.
Verdict
Approve (written verdict; posted as a comment since GitHub refuses approve/request-changes from the PR's own account). No blocking findings: all four spec items delivered at the ticket's stated seams, and every mutation claim confirmed in substance by re-execution. Non-blocking follow-ups, in order: (1) file a ticket for the install.sh/provision profile-target mismatch — the cross-writer dedupe only holds when both resolve to the same file; (2) the PR body's "exactly the landing test red" exactness is refuted (the pin test co-fires) — worth a correcting comment so the record stays honest, given this map's history with overclaimed guards; (3) Standards low findings: the dead lowercase $profile filter arm and PROFILE's missing local. Not merging, per the run's steer.
1 participant
Closes #166.
The four follow-ups from the re-verdict on #164, worked once. Every new or changed guard is proven by mutation; the selections are stated below and in the commit messages.
1. Shim-proofness is behavioural now
The "probe never executes the candidate claude" guard was a text scrub, and the reviewer's mutation — a probe executing
"$(command -v claude)"— walked straight past it. The guard is now a real run against a claude that records being invoked; the record must stay empty, and the answer is pinned tolendableso a probe that crashed early cannot pass with a trivially empty record. The recorder is shell builtins only (:and a redirection), honouring the reviewer's constraint: the probe's stripped PATH carries nothing butreadlink, so a marker needing an external binary would fail to record the very invocation it exists to catch.Mutation: the reviewer's exact one — a probe line executing
"$(command -v claude)". The behavioural test goes red; the text scrub stays green, which is why the scrub no longer stands alone (it is kept as a complement, docstring rewritten to what it actually guards).2. Profile-mark tags are derived, not hand-picked
Nothing kept the
# devlaunch: <tag>marks distinct; two lines under one mark silently drop whichever is appended second, everything exiting 0. The tag is now twelve hex characters of the guarded line's SHA-256 — derivation over assertion, per the ticket — so two different lines cannot share a mark, and identical lines sharing one is the dedupe itself. Profiles marked by an older devlaunch gain one duplicate PATH entry, once (the cost the module already accepts for any mark change).A behavioural test runs every rendered append from all three writers into one profile, twice, and requires each PATH line exactly once.
Mutations (two): the reviewer's tag collision (transfer's tag set equal to provision's) under the old code, and the derivation forced to a constant under the new — each turns exactly the landing test red while every other test stays green, matching the ticket's "77 passed" claim.
3. The feature installer carries the same guards
.devcontainer/claude-code/install.shwas excluded from the #164 sweep because it "guards its own line in its own file". The record is corrected:$TARGET_HOME/.profileis the user's profile, and its guards substring-matched.pixi/binandpixi/envs/claude-shim— latent, not live, on the stock base image. Its two edits are now_profile_prepend's rendered output verbatim, marks included, so the installer (image build) and the provision script (up) recognise each other's work instead of duplicating it; a test pins the fragments byte-for-byte and names the regeneration path. The guard property tests extend their parametrization to the installer.Mutation: the pre-change installer itself — its substring guards fail both the owned-mark and the whole-line property tests, which were red before the rewrite.
4. Vestigial assertions deleted
The two
startswith("grep -q")assertions pinned the pre-#164 guard shape;grep -qis a prefix ofgrep -qxF, so they passed while guarding nothing the whole-line property test does not. With the assertions gone the enclosing methods said only "the scripts append to the profile", which the shared guard extraction already refuses to leave vacuous, so the methods went whole.Deliberately untouched, per the ticket:
attach_workspace/setup_hostname(#168 owns the cold-path fold).Local
pixi run ci: 1447 passed, ruff and pylint clean.🤖 Generated with Claude Code
Summary by Sourcery
Strengthen shim-proofness of the tools probe with a behavioural test, and make profile PATH guards collision-safe and consistent across scripts and the devcontainer feature installer.
Bug Fixes:
Enhancements: