Detect BMAD at Module granularity during install, and never fail the install for it - #371
Draft
Hidden-History wants to merge 9 commits into
Draft
Hidden-History wants to merge 9 commits into
Hidden-History wants to merge 9 commits into
Conversation
The installer said nothing about BMAD, so a project carrying most of BMAD but missing the BMM Module looked identical to a fully-equipped one. The gap only surfaced later, as a failed dispatch, with nothing at install time to point at. Detection now resolves three distinct states rather than present/absent: no BMAD at all, a BMAD root without the BMM Module, and BMM installed. The absence message names BMM as the required Module instead of referring to BMAD in general, so an operator knows which piece to add. Presence is silent — a reassuring "detected" line is treated as a defect, not a courtesy. BMAD is not a prerequisite. Its absence is a supported operating state, so the detection path cannot change the install's exit status: the detector carries its answer on stdout and returns zero unconditionally, and the call site consumes it in a condition so nothing can escape into errexit and abort a run mid-way. The result is not written anywhere, which keeps installing BMAD afterwards from requiring a reinstall. The surface matters and is chosen deliberately. Skill directory names carry no module segment, so they can answer "BMAD?" but never "BMM?". The BMAD root config is not a module registry — it names a subset of installed modules, so reading it reports BMM missing on a machine that has it. The module tree is used instead, discriminating on the Module's own config rather than a bare directory. The absence message deliberately carries no upstream source and no version string; that half of the operator's next step is owned separately.
Module detection treated "the BMAD directory could not be read" as "the BMM Module is not installed". A directory present but not traversable satisfies the existence test while every test beneath it fails with EACCES, which the shell cannot distinguish from the file not existing, so an operator whose machine has BMM was told BMM was missing. That is the same inverted report the detector already rejects the BMAD root config for producing. Unreadable evidence is now reported as its own state, saying the availability of BMM is unknown and explicitly not that it is absent. Detection still never changes the installer's exit status. Also in this change: - A missing project-path argument no longer aborts the caller under `set -u`; it resolves to the undetermined state rather than silently answering about the current directory. - The module is certified from a non-empty config file, so a zero-byte config no longer certifies a half-written install as present. - The state dispatch has a default arm, so an unrecognised state is reported instead of producing the silence that means "present". - The comment citing the failure-taxonomy decision claimed it applied only by analogy. It binds every detector, and this is one. Corrected, keeping the narrower claim that was accurate: its enumerated fail-open list does not contain this condition. The neighbouring citation for the absent-registry decision is by analogy and stays so. - The comment claiming the guarded call site added a second, independent guarantee was wrong: invoking a function as the left operand of `||` suspends errexit inside its body, replacing that guarantee rather than backing it up. The trade is stated instead of misdescribed. - Portability wording no longer claims containment it does not have: the tests follow symlinks, which a shared BMAD install relies on. Tests: the module goes from 23 to 34 cases, including the states above, a no-argument call, a regular file where the BMAD root is expected and a dangling module symlink. The harness now observes a non-zero return instead of aborting before it can be read, environments are pinned so an exported log level cannot flip results, and fixtures that cannot construct unreadable evidence fail loudly rather than skipping green. Installation notes now show the actual message text and stream, cover the empty-module and undetermined states, and scope the exit-status claim to this check rather than to the installer as a whole.
…lity guard The Module-presence check accepted any non-empty path at the config location, so a directory in place of config.yaml (a botched merge, an aborted extraction, a mkdir typo) was reported as the Module being installed. The check now also requires the path to be a regular file. The BMAD-root and Module-directory readability guards required both read and search permission, but the detector only ever performs named lookups on known children, which need search permission alone. On a directory that is searchable but not listable, the guard reported detection as undetermined even though the BMM Module was present and fully resolvable, incorrectly emitting a warning on a repository that has BMM installed. Both guards now require only search permission, with a comment explaining why read permission isn't needed. Added tests covering both cases: a directory in place of config.yaml, and a search-only BMAD root with BMM present.
…sent The Module detector concluded "bmad-absent" whenever the directory test on the _bmad path returned false. That test cannot distinguish "there is no _bmad" from "a parent directory refused us", so three reachable layouts reported a confident absence on machines that do have BMM installed: a project directory the installer cannot search, a _bmad symlink whose shared-install target cannot be resolved, and a self-referential _bmad symlink. All three now resolve "bmad-indeterminate", which is what the detector already answered for an unreadable _bmad itself. The guard is a disjunction because neither half covers the other: a symlink test catches the link cases, and a search test on the project path catches the case where _bmad is a real directory and the symlink test is blind to it. A project path that genuinely does not exist still reports absent - there the answer was looked up and found. The undetermined message told the operator to check that _bmad is "readable". The gate is search permission, not read permission, so a directory at mode 444 - readable and listable but not enterable - named the one permission that was not the problem. It now names entry permission, agreeing with INSTALL.md. Header comments, all describing code that had moved underneath them: the discriminator paragraph still presented the emptiness test as the single test after the node-type test was re-added alongside it; the escalation receipt cited no decision record and dropped half of the ruling it paraphrased; AD-36 was cited without the analogy marker its Binds line requires; and the fourth state was described as a general "could not look" detector when what it detects is permission denial observable from the project path downward. The residual above the project path is now stated instead of implied. The knowing exception to the no-warning criterion is declared where a reader re-derives that criterion. Tests: one fixture's fail-loud guard probed a config.yaml the fixture never created, so it could never fire and a filesystem that does not enforce modes reported green instead of refusing to measure. The reporter's capture form had no test at all, so rewriting it in a way that masks a failing detector left the suite green; the new test uses a bare call, which is the only shape that can observe the difference.
The guard added in the previous commit fired on any symlink at _bmad whose directory test failed. That was too broad. A symlink pointing at a regular file, a FIFO, a socket or a device resolves perfectly well - the answer is "we looked, and it is not a BMAD root" - but it was reported as undetermined, while a bare regular file at the same path was correctly reported absent. The identical situation got opposite answers depending only on whether a symlink stood in the way, which is the defect this guard exists to remove, one node type over. The guard now tests whether resolution succeeded rather than what the target turned out to be. Testing for a regular file instead would still misreport a FIFO, a socket and a device, all of which resolve. Dangling links, symlink loops, and links whose target cannot be searched stay undetermined, and they must: all three present identically here, because the shell's file tests expose no errno and cannot separate a missing target from an unreadable one from a loop. One of the three is a permission denial that has already been ruled undetermined, so the other two take that answer rather than being guessed apart. Both previously unpinned directions now have tests, and the three candidate forms of this guard are distinguishable by them: reverting to the broad form fails the regular-file case, and narrowing to a regular-file test fails the device case.
…, not absent The root guard already treats a dangling, ELOOP, or unresolvable-parent symlink at _bmad as bmad-indeterminate, because [[ ]] exposes no errno and cannot tell ENOENT, ELOOP, and EACCES apart -- the adjudicated permission case rules all three. The equivalent guard at _bmad/bmm never gained that limb, so the same three shapes one level down fell through to bmm-absent, including the case where the module's own config.yaml resolves and is non-empty. An operator whose machine has BMM could be told BMM is absent. detect_bmad_module_state's module-level guard now mirrors the root's -L/! -e limb. The pinned test for a dangling module symlink asserted the old, incorrect answer; it is rewritten to assert bmad-indeterminate, renamed, and moved beside its root-level counterparts. A module-level fixture pair covering the unresolvable-parent shape is added, mirroring the existing root-level fixture. INSTALL.md and the indeterminate-state message are extended to name the Module directory alongside _bmad, since both are now sources of this state.
….yaml Same class as the previous commit, one node further down: a shared-install layout can link only the Module's config.yaml rather than the whole bmm directory, and a dangling or unresolvable-parent symlink there fell through to bmm-absent for the same reason the Module directory guard did before it gained the -L/! -e limb -- resolution failing is read as the Module not being there, on a machine where it is. detect_bmad_module_state gains a third guard, checked after the Module directory resolves and before concluding bmm-absent, using the same depth-independent standard as the two guards above it. Two tests cover the dangling and unresolvable-parent shapes at this node, mirroring the existing Module-directory fixture pair.
The dangling/unresolvable-symlink guards added at _bmad/bmm and _bmad/bmm/config.yaml mirror the root guard's -L / ! -e limb, but every existing symlink fixture at those two depths is dangling (-e false), so the ! -e half of either guard was unpinned: deleting it left all 47 tests in the module green while turning every healthy shared install into bmad-indeterminate. This is the same defect the root guard already paid for once (83d9573 -> 9d73814). Adds four tests mirroring the root's own regular-file / /dev/null discrimination pair at each new depth. The config-depth resolved-symlink fixture targets an empty file rather than a merely non-empty one: the presence check (-f && -s) runs first and follows symlinks, so a symlink resolving to a non-empty file is legitimately bmm-present.
The BMM-undetermined warning dropped the project-path clause ("it and")
that a prior round added on purpose when this round's two new node
names were appended to it; the project path is a live independent
cause of this state and needs its own pointer back.
Both the warning and INSTALL.md blamed the indeterminate state on
permissions alone, with no mention of the symlink causes these guards
exist to catch -- for a dangling or unresolvable symlink there is no
permission to fix, so that advice could never resolve it. Both now name
the dangling/unresolvable-symlink cause and its remedy (repair or
remove the link) alongside the permissions cause.
INSTALL.md's BMM-absent row also named "config.yaml is empty" without
distinguishing a zero-byte file (still absent) from a dangling symlink
(now indeterminate, not absent since the config-depth guard shipped);
reworded and cross-referenced. The undetermined row now names
_bmad/bmm/config.yaml explicitly -- the prior round documented the
Module directory guard but not the config.yaml guard shipped beside it.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
scripts/install.shnow reports BMAD availability at Module granularity rather thantreating the BMAD root as a single present/absent signal. Four states are distinguished,
each with its own named report:
Why
The previous check treated any unreadable or non-standard
_bmadlayout as "absent",which is wrong in both directions. An operator with a working BMM install but an
unreadable ancestor directory was told BMAD was missing; a symlink pointing at a regular
file was reported as a working install. Both are confident wrong answers the operator has
no way to detect.
The install must also never fail because of this check. Detection and reporting both
return zero in all four states, including undetermined -- a diagnostic about an optional
dependency is not grounds to abort an install.
Scope
scripts/install.sh-- detection and reportingtests/test_install_bmad_module_detection.py-- new; covers all four states and theboundaries between them
INSTALL.md-- documents the four states and what each means for the operatorCHANGELOG.md-- Added entryAdditive only: 1448 insertions, 0 deletions.
Notes for review
Opened as a draft. This is the first CI run against this branch -- the repository's
workflows trigger on
pull_requesttomain, so none of these commits has been exercisedby CI before now.
AC-3 ("BMM present -> no warning at all") is knowingly violated in exactly one state: a
repository that has BMM but whose evidence cannot be read resolves to undetermined and
warns. This is declared in the reporter header in
install.shand grounded in a projectdecision that postdates the criterion.