Record why Parzival is not enabled, and stop the installer losing that reason - #366
Draft
Hidden-History wants to merge 19 commits into
Draft
Hidden-History wants to merge 19 commits into
Hidden-History wants to merge 19 commits into
Conversation
…esentable The enablement record gains a cause and a condition alongside the flag, so "declined at install" and "the installer could not deploy it" stop reading as one state. Every consumer branches on the cause; a deployment failure emits an error naming it rather than only recording it. Applies the post-implementation dual review: - The record is written in a single atomic pass instead of three sequential set_env_value calls. Value-last ordering only narrowed the window in which (enabled x non-empty cause) was observable; a re-install over a .env already carrying PARZIVAL_ENABLED=true transited that cell with no interrupt at all. set_env_value itself is unchanged - it has callers far beyond Parzival. - The guard for that invariant previously read file key order on a freshly appended .env, so it exercised only the append path and passed against a value-first implementation every time. Replaced with a test that enters the replace path, sabotages the write at its commit point, and carries its own positive control. - The fail-closed cause rule had drifted across its Python and shell copies within one commit: only the Python copy normalised case, and cut -d= -f2- passed quotes and CRLF through, so the installer and the SDK could report different causes for the same file. A cross-implementation equivalence test now asserts one input table through every reader. - The v2.0.5->v2.0.6 migration no longer seeds a cause. It cannot know one, and because update_env_file appends per key, seeding any value on an already enabled install wrote the forbidden cell directly. Absent reads as unknown. - resolve_cause no longer swallows an undeclared field, and disabled_message normalises its argument instead of silently rendering the unknown message. - The two AC-2 tests that compared byte offsets and re-asserted a copied template are deleted rather than repaired; the abort window is now entered by driving the real copy_files. - TR-7 fixture pairs added for the five converted sites that had none, and the cause-invariant site now asserts its invariance across all three causes. - docker/.env.example shipped PARZIVAL_ENABLED=true, contradicting the documented default and leaving a green flag over an undeployed component if an install aborted after the template was copied. Now false. - The Manual Enable procedure told operators to set the flag without clearing the cause, producing the forbidden cell by documented procedure. - Three documents published three different defaults for one key; reconciled. - EOF on the interactive prompt recorded cause=opt-out, reporting an environment condition as an operator decision. It now records no cause. Also fixes a live test leak that was writing real classification-queue records into a repo-root MagicMock/ directory, and gitignores machine-local .sot state that would otherwise have been committed. Story tests 49 -> 152. tests/unit and tests/hooks: 1771 passed.
…m to what was measured The two-signal re-derivation was recorded in the round-2 notes but not in the section the finding named, so the consumer-set table still showed 13 hand-written rows. The four documentation surfaces it found are now rows in that table, marked with which signal reached them. The 9p note claimed more uncertainty than the evidence supports, and rested on testing that never touched 9p: pytest tmp_path resolves under /tmp, which is ext. Measured on the real v9fs mount, mv carries the inode across, so it is a real rename(2) and a concurrent reader cannot observe a half-written file. What remains open is durability across a host crash, not visibility - a smaller claim, now stated as such. Also records that chmod --reference is guarded by || true, so a failure silently leaves mktemp's 0600 rather than the original mode. The upgrade.sh block extraction now asserts its lines are contiguous. It assumed a single block; a PARZIVAL_CAUSE= assignment appearing elsewhere would have spliced unrelated lines into the normaliser and quietly changed what the test exercises.
…t cause Installer - Treat a populated answer as an answer regardless of read's exit status. read returns non-zero on EOF even when it has already filled the variable, so `printf 'y' | install.sh`, a heredoc without a trailing newline, and expect drivers all had the operator's "y" discarded and a decline recorded in its place. - Write nothing to the record when the prompt reaches EOF with no answer. The previous write overwrote a correctly recorded deployment failure with an empty cause, and because setup_parzival never reads the existing PARZIVAL_ENABLED it disabled a deployed, working install because nobody answered a prompt. This also restores install.sh to the four false-write sites and one true-write site the record's specification enumerates; the EOF site was never among them. - Match export-prefixed and indented keys when rewriting the record. An anchored pattern missed `export PARZIVAL_ENABLED=true` and appended a second definition, leaving the shell and python-dotenv reading different values from the same file. - Guard creation of the record file. It was the only unguarded failure path in the writer, and under set -e an unwritable docker/ turned "Parzival is off" into a failed install. - Transfer file mode and ownership portably. chmod --reference and sync FILE are GNU-only while macOS is supported, where the rename silently committed a 0600 temp file over docker/.env. - Give the opt-out summary panel its re-run clause, so following the advice literally no longer leaves an enabled flag beside a stale cause. - Stop the fallback arms in install.sh and upgrade.sh claiming that no cause was recorded. The enablement gate is case-sensitive, so those arms are also reached on installs the SDK considers enabled; they now report only what they can determine. Settings transport - Delete PARZIVAL_ENABLED from settings.json when Parzival is disabled rather than writing false, while still carrying the cause and condition. That env section reaches the hook process and outranks docker/.env, so a persisted false overrode the very file operators are told to edit. - Remove a stale cause on the enabled path, which the disabled path already did. Consumers - Guard the memory.parzival_state imports in bootstrap.py and manual_save_memory.py, and stop an import failure being reported as a config failure. bootstrap.py's output is injected into a session, so an unguarded import delivered a traceback where a message belonged. - Correct the shipped CLAUDE-PARZIVAL-SECTION.md default and the cause table row that claimed an absent cause means the install predates the record. Tests - Cover the four installer fixes, each demonstrated failing beforehand. - Assert the settings precedence rule directly rather than describing it. - Add the missing cause-discriminating pairs for injection.py and bootstrap.py, and pin per-cause observables where an assertion was green for every cause. - Remove the re-typed normalisation arm from the equivalence harness and compare the two shell implementations directly, cover the fourth reader, and record the forms the table cannot express. - Document where spec-bound mocks stop helping, and add a guard that fails on a misspelled record field. - Assign the record fields in a test that had been passing on the wrong branch because a blanket handler swallowed the deliberate error.
Scope the stale-key delete to the state variables so the five preference variables are no longer dropped silently on an update. Correct the disabled-state panel arm to point at the enablement record instead of asserting that no cause is on file. Reorder chown before chmod so an ownership change cannot clear the mode bits, and warn when the mode cannot be read rather than skipping the chmod in silence. Remove an operand-less sync fallback that flushed the whole host to write three lines. The test changes make several assertions capable of failing. The static guard's control now drives the real detector rather than re-implementing its regex; the typo exemption is scoped to its own module instead of being self-service; two absence cases are constructed by removing the attribute rather than assuming it away; and both import-fallback branches are covered for the first time. 11 items applied. Suite 205 to 211 passed, no regressions.
…sions set_parzival_enablement() copies owner and mode from docker/.env onto a mktemp file before renaming it into place. Both captures used a BSD stat fallback that is not a fallback on GNU coreutils: `-f` there is --file-system, so `stat -f '%Lp' <file>` treats '%Lp' and the file as operands, fails on the first, and still prints a multi-line filesystem block for the second. The command substitution captures that block. The guards tested the captures for emptiness, which is true of a filesystem dump. On the one path these blocks exist for -- the primary stat failing -- chmod was handed 332 bytes of "Block size: ... Inodes:" text, failed into `|| true`, and the rename published mktemp's 0600 over docker/.env. The warning that exists to make that case loud never fired. Measured end to end on GNU coreutils 9.4: 0644 in, 0600 out, silently. Both captures are now matched against the shape of the value they are meant to hold, so a successful read is distinguishable from a read that returned something. The mode pattern allows one to four octal digits because GNU %a strips leading zeros -- mode 0044 prints as 44 and 0004 as 4, and a three-digit floor would reject a legitimate mode and skip the chmod it guards. Also narrows the success panel's disabled-state arm to report only not-enabled and point at the record rather than characterising why, which keeps both lines true whether the record is absent, empty, or present with an unmatched value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
set_parzival_enablement() copies owner and mode from docker/.env onto a mktemp file before renaming it into place. The transfer has two attributes and two failure stages, so four ways to fail: each capture can be unreadable, and each syscall can be refused with a well-formed value in hand. Only one of the four said anything. The owner arm had no branch at all. Written as `[[ regex ]] && chown ... || true`, the trailing `|| true` is reached both when the guard rejects the capture and when chown itself fails, so the two outcomes were indistinguishable and silent. Both arms are now if/else with a message per stage, naming what was lost and what the record keeps instead. Neither message claims a cause. The syscalls keep 2>/dev/null: the warning is the operator channel and raw stderr from a best-effort probe is not. log_warning is an echo, so the function still always returns zero -- a record that could not take the file's permissions must not become an installer that dies. Also tightens an assertion that was passing for the wrong reason. It matched "Could not read" and "mode" as two independent substrings; with the mode arm silenced it still passed, because "Could not read" came from the owner message and "mode" came from pytest's tmp_path, which is derived from the test's own function name. It now matches the full phrase. Tests cover the three previously silent paths and a control asserting a healthy install stays quiet. The control cannot be red before the change -- it asserts absence, and the strings did not exist -- so it is proven by inverting a guard to warn unconditionally and observing it fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The five log_* functions were each a bare `echo -e`. set -euo pipefail is global and main() redirects stdout into tee, so every one of them runs with its stdout owned by another process. An echo that cannot write returns nonzero, errexit sees a failed simple command, and the installer dies -- losing whatever the next line was about to commit. The logger was deciding the exit status of the program. Measured with stdout closed ahead of the enablement record write: the installer exits 1 and the record is never written. Guarding the echo with `|| true` returns it to exit 0 with the record committed. The obvious alternative does not work and is worth naming, because it passes every existing test while shipping nothing: appending `return 0` after the echo leaves errexit firing AT the echo, so the return is never reached and the run still dies. Measured identically to no fix at all. The guard is not total, and the comment says so. `|| true` suppresses errexit, so it covers a write that fails and returns -- a closed descriptor, ENOSPC, EIO. It does not cover SIGPIPE: with no reader left on the pipe the kernel kills the shell outright, measured as signal 13 with the record uncommitted, identically with and without the guard. A signal is not an exit status and no `||` clause runs after one. Covering it needs a trap at script scope, which changes signal disposition for the whole installer and is deliberately not done here. The fix is scoped to the five definitions rather than to call sites; there are 486 bare log_* statements and the failure belongs to the function, not to any one caller. Nothing consumes a log_* exit status as a condition: all 11 uses in a conditional position have log_* as the consequent of `||`, never the test. Also corrects a skip guard that asked the wrong question. The unreadable- owner test was gated on a probe of `stat -f '%Lp'`, a mode format, while its reason asserted that the owner fallback succeeds. The two co-vary on every platform in use today, which is what kept it invisible: the gate returned the right answer for the wrong reason. It now probes `stat -f '%u:%g'` and matches the owner shape the installer itself matches. The negative control's docstring no longer claims its four absence assertions are backed unconditionally -- two of the four rest on tests that skip where those stat formats are real. The unappliable-mode test asserted its message and not its result. Its sibling already checked that the committed file degrades to mktemp's 0600; this arm reaches the same end state through the apply cell rather than the read cell and now asserts it, so the message is no longer the only evidence for what it claims. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous change guarded all five log_* functions so a dead stdout could not end the install. That fixed a loud abort and created a silent success. set_parzival_enablement always returns 0 by contract, so its four write-failure branches announced themselves through log_error and through nothing else -- and log_error is now a guarded echo that drops its message when there is no stdout to write to. main() redirects into tee, so stderr is not a fallback channel. All four branches could therefore reach `return 0` with nothing delivered anywhere, and nothing counted the failure: every *_count in the file is a local for an unrelated purpose. The installer exited 0 having printed nothing, which is indistinguishable from a healthy run. Two channels answer it, and neither replaces the other. An installer-wide counter is incremented at each of the four branches and drives `exit 3` from a gate at the end of main -- the exit status is what sends an operator or a CI job to look at all. A durable append to $INSTALL_DIR/parzival-record-failures.log gives them something to inspect once they do, because an in-memory count dies with the process. The increment is placed before both the log_error and the append. Both are established as failable, and a count taken after either one is a count a broken channel can suppress. `return 0` is untouched at all four branches. Reporting the failure and aborting the install stay separable, which is the existing contract; what stops being separable is failing and saying nothing. The gate is the last statement in main, after show_success_message, and a second global is set immediately before it. Without that global a bare `exit 3` makes the EXIT trap print "Installation interrupted" and "Partial installation exists" over a complete install, replacing one lie with another. show_success_message is deliberately not touched. It carries 66 unguarded echo lines, and today those are the only thing producing any failure signal at all, because a run with a dead stdout dies there under errexit. Guarding them before this gate existed would have removed that accidental signal and made the silent success universal. That sweep lands with or after this change, never before -- and it is also why the durable file is not optional, since a counter that can only speak through a gate the run never reaches is a counter that says nothing. Three details that would each have re-created the defect inside its own remedy, all measured rather than reasoned about: The append is guarded. An unguarded >> that fails under set -euo pipefail aborts the install, which is precisely the bug being fixed. Measured: the unguarded form exits 1 and the following line never runs. The counter uses `x=$((x + 1))`, not `(( x++ ))`. The post-increment operator evaluates to the old value, so under errexit the very first increment of a zero counter returns 1 and kills the install. Measured: the operator form exits 1 with the next line unreached; the assignment form exits 0. The `2>/dev/null` precedes the `>>` rather than following it. Redirections apply left to right, so in the usual order bash reports the failed redirection on the original stderr and the suppression never takes effect. The write target is neither docker/.env, which is unwritable by hypothesis and is the failing write itself, nor $INSTALL_DIR/logs/, which main creates with `mkdir -p ... || true` and may therefore not exist. $INSTALL_DIR survives both: it is created by an unguarded mkdir -p so a failure there aborts the install long before this code runs, it sits one level above docker/ so the permission problem does not reach it, and unlike /tmp it is durable across a reboot. Each ledger line carries its own timestamp rather than the file being truncated on start. Truncation loses the history across repeated failures, which is what diagnosing an intermittent write failure needs, and without run identity the ledger reproduces the very defect this story exists to fix: run 1 fails, run 2 succeeds, and the operator reads a file that still names a failure. What persistence buys is bounded and the comment says so: where the trigger is a full disk the append fails too. This recovers the permission case, not the ENOSPC case. AD-24 and AD-26 are cited by analogy only. They live in a different spine and their Binds: scope them to the resolver and dispatch path, not to the installer write path; set_parzival_enablement's own contract comment is what makes them local precedent. A literal binds-match claim would be false. On the tests: the log-guard regression covered one of the five functions. log_info appeared once in the module and the other four log_* names zero times, so four guards could have been deleted outright with the suite green. The case is now parametrised over all five, each demonstrated red by deleting exactly that one guard. log_debug carries LOG_LEVEL=debug because its echo is inside a level branch and would otherwise never execute, passing identically against an unguarded body. What that test drives is stated in its docstring, because it is not the production trigger. The mechanism is a descriptor the test closes itself, which no site in install.sh can produce. The real breakage is tee dying, arriving either as a write that fails and returns -- the class the guard covers, and which the closed descriptor stands in for -- or as SIGPIPE, which the guard does not cover and no case asserts. Contract change, stated here rather than left for a red pipeline to discover: tests/integration/test_installation.py asserts returncode == 0 and two jobs in test-installation.yml run install.sh bare. A genuine record failure will now fail CI. That is the intent.
… fail The mechanism shipped in the previous change is correct. Its own test surface could not detect that mechanism's removal, in three places, each measured rather than reasoned about. The gate's only wiring into the product -- INSTALL_COMPLETED=true followed by parzival_record_status, the last two statements of main -- was executed by no test. install_sh_no_main strips `main "$@"` so main never runs, and every gate case calls parzival_record_status directly with the flag hand-set. Deleting both lines left the module at 40 passed: the gate was removable from the product while the suite attested it was there. A text-level assertion over the shipped file now holds their presence, order and position, in the idiom install_sh_no_main and the integration tests already use. It cannot prove the lines execute and does not claim to. It does fail on every variant of the mutation that shipped green -- both lines deleted, either deleted alone, or the pair moved above show_success_message. Only the first of the four counted write-failure branches had a count assertion. The pre-existing tests over the mktemp, awk and mv branches assert message text, which parzival_record_failure re-emits verbatim through log_error "$detail", so they pass whether the site counts the failure or not. Reverting the commit branch -- the one that fires on a read-only remount, EDQUOT or a cross-device rename -- to a bare log_error left the module green. Each branch now has a count-asserting case, driven by shadowing the command under test with a shell function, which is deterministic and behaves identically for every uid. A case joining a real write failure to the real gate in one shell is added; nothing joined them before, so the count the failure produces and the count the gate reads were the same variable only by assumption. The sole test holding the ledger append's `|| true` drove failure with chmod(0o500) and carried no root skip. Under uid 0 the append succeeds, the guard never fires, and the case passes with the guard deleted. Its class docstring asserted in writing that no case in it could go vacuously green under uid 0 -- true of the missing-parent cases, false of that one, and stated where a reviewer reads it before deciding whether to look. The chmod case now carries the same skip as the precedent in this file, the docstring is scoped to what it covers, and a sibling case drives the same failure through a missing parent directory, which ENOENT makes unwalkable for root. Verified: with the guard deleted that sibling fails alone, so the guard stays held where the skip fires. No behaviour changed. Three comments that shipped false are corrected. The write-target justification claimed $INSTALL_DIR is created by an unguarded mkdir; that mkdir is in create_directories, whose only call site is inside main's full-mode block, so in add-project mode -- the default -- $INSTALL_DIR and logs/ are created by the same guarded mkdir and fail together. The offered discriminator does not discriminate; the target stands on its other two grounds, and the false ground is recorded rather than deleted so it is not re-derived. The ordering note placed a dead-stdout death at show_success_message; main emits 11 line-initial unguarded echoes immediately after the tee redirect and step() fires two more on every phase, all before setup_parzival, so such a run dies at the banner. The case the gate actually recovers is the permission case with healthy stdout, which exited 0 before and exits 3 now. The exit-code header gained the 3 the previous change introduced. show_success_message is byte-identical, sha256 94088a6c5320efe0. The TD-1082 ordering constraint is unchanged.
…iew) Adds a per-capability degraded declaration to the pov-tree skills and workflows that depend on BMAD, a resolver that distinguishes an absent registry from an empty or unreadable one, and a discovery function that derives the declared set from the sites themselves rather than from a maintained list. The cwd sentinel no longer treats a missing BMAD installation as workspace drift: it reports the dependency as unavailable and exits zero, so a dispatch that never needed BMAD is no longer aborted with the wrong cause. The BMAD dispatch path gains a gate that stops before creating a pane when BMAD is absent. Not complete. Two review rounds returned changes-required and the work is committed as a recovery point, not as a finished story. Known outstanding: discovery raises on an unreadable declaration site; an undecodable registry reports as resolved; three declarations cite tests that do not exercise what they declare, and the guard added to catch that excludes by filename and so forbids the one correct citation. Four dependent paths sit outside the walker and cannot be declared at all. CHANGELOG.md carries two lanes' work; this commit contributes two lines to it.
Owner
Author
Known defect on this branch, with the fix already specifiedFlagging before review so it is not rediscovered. The defect. Scope. This is confined to this branch. Why it is worth stating now. The existing tests assert on the emitted token, not on which signal produced it, and every fixture seeds both signals together — so no test in the suite fails on this. It would merge green. Status of the fix. The acceptance criterion this implements ( No action required on this PR. If it merges first, the defect becomes a regression on |
added 9 commits
September 1, 2026 16:39
Three capability declarations pointed at tests that did not assert the behaviour those declarations describe. A citation that cannot fail when the declared behaviour is removed is not coverage, so each is either re-pointed at a test that does assert it or marked as unenforced. - aim-agent-dispatch and aim-model-dispatch now cite fixtures that assert the gate reports the dependency as unavailable, continues on the non-BMAD path, and terminates. Both assertions were confirmed to fail when the behaviour they cover is deleted. - aim-agent-lifecycle is marked not-yet-enforced. No fixture asserts its declared behaviour today, and an inaccurate citation is worse than an explicit record that none exists. The self-citation guard is narrowed from a broad pattern match to a default-deny allowlist, so a declaration citing this module is refused unless it is admitted by name. One entry that met no stated criterion is removed, and a test that returned an identical verdict for all sixteen declarations is deleted, with the criterion it failed restated where the allowlist is defined so it is not reintroduced. The new fixture asserts every gate site rather than one chosen by position, so a later gate that omits the declared behaviour is caught.
Both BMAD-presence gates in the agent-dispatch skill reported the missing dependency but not what would supply it, so an operator on a machine without BMAD was told what was wrong and not how to fix it. The sibling dispatch path already names the upstream source in the same situation. Both gate sites now point at DEPENDENCIES.md, and the fixture that reads them asserts the remedy half rather than only the dependency half. The assertion was confirmed to fail against the previous prose.
The agent-lifecycle capability was marked as having no test on the grounds that no fixture asserted its behaviour. That is not the test the marking rule states: the unenforced marking is unavailable wherever a compliant fixture can be built, and one can be built here. The capability is the structural twin of agent-dispatch, which was cited to a fixture in the same change that marked this one unenforced, and the asymmetry had no stated reason. Its Step 1 gate already stated three of the four things the declaration claims. The fourth, what would provide the missing dependency, is added in the same form the sibling skills use, and the declaration now cites a fixture that asserts all four at every gate site. Removing any of the three behavioural clauses was confirmed to fail the cited test, so the citation is coverage rather than a checkmark.
The test was called test_every_declaration_names_its_dependency_and_remedy and its docstring cited AC-1, but it only resolves the join between a capability's declared dependency and the entry that supplies the remedy. It never reads the message a capability emits, so it stayed green when a declaration's behaviour text was replaced with a placeholder naming neither the dependency nor the remedy. A test whose name claims a criterion it does not check invites exactly that citation, so it is renamed for the join it verifies and its docstring now records what it does not cover and where that coverage actually lives.
…er guard The agent-dispatch cycle's spawn step told an operator on a machine without BMAD what was missing but not what would supply it, so the message carried three of the four things a degraded capability owes and still read as complete. The gate line now names the remedy, and cap:cycle-agent-dispatch cites a test asserting all four clauses at every gate site instead of declaring itself unenforced. The guard against the superseded three-marker sentinel matched a single literal command string. That string had already been removed everywhere, so the guard was green while the same rule survived as prose at sites it could not reach. It now enumerates every line in the pov tree naming the _bmad directory, requires each to appear on a declared exemption set with a stated reason, and fails on anything else, so a new phrasing arrives as a hit somebody must disposition rather than passing silently. A companion test fails on an exemption that has gone stale and no longer matches anything. Six sites asserted a three-marker expectation the sentinel stopped enforcing. Both cwd_sentinel.sh failure messages promised all three markers while the check they report on tests only two, so an operator genuinely in the wrong directory was told to fix a marker that was never required; the two success-criteria lines and the team-builder template made the same claim. All six now describe what the code does. tests/test_cwd_sentinel.py pinned the exact text of one of those failure messages and is updated to match.
The guard enumerating pov-tree lines that name the _bmad directory asserted
only that its undeclared list was empty. An empty list is also what a walk that
collected nothing returns, so narrowing the enumeration -- to one subdirectory,
to *.md only, or to a path that does not exist -- left the guard green. The
enumerator now returns what it collected alongside what was undeclared, and the
test asserts collection is non-empty and reached every file a declared
exemption names. A tmp_path pair drives the real enumerator over a seeded tree,
so the guard is observed refusing rather than assumed to.
The predicate matched the _bmad token only, so a stale three-marker mandate
written test -d "${bmad_dir}" was collected nowhere. It now also matches the
bmad_dir variable that holds the same path, taking collection from 24 to 28.
The four newly collected lines in cwd_sentinel.sh are declared, with
deliberately long fragments: an exemption licenses a file plus a fragment
rather than a line, so a short one would rescue text appended beside it.
The cycle dispatch gate test asserted three clauses on the gate line but not
that the gate was mandatory, so downgrading MUST to MAY passed, and so did MUST
NOT. It now asserts the imperative. Termination is deliberately not asserted:
the capability declares that it runs the generic spawn and reports the
activation step as unavailable, so fall-through is its declared behaviour and
asserting an abort would assert the opposite.
The team-builder dispatch plan told an agent that a null bmad_agent_type is a
FAIL wherever a BMAD role fits, with no condition for BMAD being absent -- the
opposite of that capability's own declared degraded behaviour. It now names the
absent case.
test_cwd_sentinel.py's docstring quoted the superseded three-marker command,
wrapped across two lines where a line-based search cannot see it, and cited
line ranges that had since moved. Both are corrected.
The guard reported green in three states where it should have refused. Each is fixed in the order its correctness becomes observable. Walk integrity. os.walk swallowed directory errors and read_text swallowed OSError, so an unreadable subtree left the enumeration silently short and returned exactly what a clean tree returns. A seeded three-marker offender under a chmod 000 subtree was reported as a clean pass. os.walk now gets an onerror that raises, read_text no longer catches, and the enumerator returns the number of files it visited so the shipped call site can assert a floor against it. Every other count in that test is taken over the same walk, so without this they were measurements of a tree that may never have been read. The floor is 400, measured from the walk itself: it visits 542 files in a fresh checkout and 585 in a live worktree. It is a floor rather than an equality because an equality fails on every ordinary file addition, gets bumped without thought and stops being read. The largest single subtree holds 287 files, so losing any one of them lands below it. Falsifiable disposition. The positive control seeded a line containing a superseded claim verbatim, so it left the enumerator at the superseded branch and never reached the exemption predicate. That predicate was consequently executed by no test: replacing it with an unconditional skip, waving through every offender in the tree, left the whole file green. Two further probes now cover the other two exits -- a marker line at a non-exempt path, and a line at an exempt path that the exemption's fragment does not cover. The docstring claimed the missing coverage and has been corrected. Exemption breadth. The "bmad_dir=" fragment was nine characters and licensed every line in the sentinel containing that string, including a line carrying a stale three-marker mandate alongside the assignment. It is replaced by the two full assignment lines it was standing in for. The mandate written through the variable rather than the literal path carried no superseded claim and passed; both shell spellings of it are now listed. Known and unchanged: the 28 exemptions remain hand-judged, and the staleness guard still only checks that each matches something, never that it matches only what it should.
Every fix below closes a surface that could be destroyed with the whole module still passing. The superseded-claim branch was observed by nothing. Its probe was seeded at a non-exempt path, where that branch and the fall-through both append to the undeclared list, so the assertion held either way: deleting the branch outright, or introducing a typo into any one of the four _SUPERSEDED_CLAIMS entries, left the module green. The probe now sits at an exempt path on lines the path's own declared fragment matches, so the branch has to take precedence over an exemption that would otherwise skip the line, and it seeds one line per entry so no entry is left unwatched. The claim text is copied literally rather than generated from the list under test, with an assertion holding the two copies in step: a probe built from that list moves with it, and a typo then corrupts both halves and stays green. A symlinked subtree evaded the walk entirely. os.walk does not descend into one by default and raises nothing, so the onerror handler could not see it and an offender inside a symlinked subtree was missed with the enumeration reporting clean. The walk now follows links. Latent here -- the pov tree carries no symlinks -- so this closes a coverage claim rather than a live hole. The positive control had stopped asserting where a line was reported. Renumbering every reported line 0-based left it green, and a report without a location cannot be acted on. It now compares whole records. Probe C's discriminating property was asserted by nothing: retargeting it off the exemption set turned it into a duplicate of probe B, both exiting the same way. The property that a file is licensed for a fragment and never wholesale is now asserted on what the enumerator reported. A repo-local TMPDIR breaks it for real, not only under mutation. The file-count floor is kept and its justification rewritten, because the old one credited it with saves it cannot make. A narrowed root is caught by the exemption-coverage check, non-empty at all nine single-subtree narrowings while the collection check goes empty at seven. An unreadable subtree is caught by the walk's error handler, which raises: chmod 000 on workflows/phases, 101 files, leaves 441 visited -- above the floor -- and reports clean on every other instrument, so the floor would not have caught it either way. Nor does losing any one subtree land below the floor; only workflows does, at 255 remaining, because the slack is 142 files and skills is 132. What the floor does catch is narrow enough to be worth stating exactly: a loss that is silent, larger than the slack, and sparing every file the other instruments name. Deleting 286 files from workflows/ while keeping the one exemption file it holds leaves visited at 256 with collection, disposition and coverage all clean -- the floor alone refusing. That is the case it is now documented for, and no other.
The floor comment and the enumerator docstring credited the error handler with catching an unreadable subtree, and the followlinks argument with catching a symlinked one. Both are present in the code and neither is exercised by any test here: the handler has one call site and no fixture makes it fire, and the tree carries no symlinks. Both are now described as reasoned and demonstrated by hand at authoring time, but unobserved by the suite. No coverage is added and no behaviour changes; the parsed module is unchanged.
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.
Records why the Parzival session agent is not enabled, instead of only that it is not, and stops the installer losing or misreporting that reason.
The problem
docker/.envrecordedPARZIVAL_ENABLED=falsewith no indication of why. An operator who declined at the prompt, an operator whose deployment failed, and an operator who was never asked all produced an identical record — so a broken install and a deliberate opt-out were indistinguishable, both to the operator and to every consumer that reads the flag.Several paths also corrupted the record while writing it:
readreturns non-zero on EOF even when it has already populated the variable, soprintf 'y' | install.sh, a heredoc without a trailing newline, and expect-style drivers had the operator's answer discarded and a decline recorded instead.falsewith an empty cause over the shared record, overwriting a correctly recorded deployment failure and disabling a deployed, working install because one add-project run had no stdin.export PARZIVAL_ENABLED=trueand indented keys were missed and a second definition was appended — leaving the shell andpython-dotenvreading different values from the same file.chmod --referenceandsync FILEare GNU-only while macOS is supported, so the rename could silently commit a0600temp file overdocker/.env.What changes
read's exit status.export-prefixed and indented keys, guards file creation, and transfers mode and ownership portably.Testing
Story-scoped and touched-surface modules pass (270). Regression run over the CI-scope suite completed with every remaining failure class reproduced at the base commit; the one test that failed uniquely because of this change was root-caused — it had been passing on the wrong branch of a blanket exception handler — and fixed by correcting the fixture rather than loosening the guard. Local
ruff,black --checkandisort --check-onlyare clean, as arebash -nandshellcheck -S erroron both shell scripts.Note that the local virtualenv is drifted from the declared pins, so the local gate is not equivalent to CI. CI is the authority here.
Not ready to merge
Opened as a draft deliberately. Outstanding:
src/memory/injection.pyimports the cause resolver unguarded at module level, andsync_installed_filescopiessrc/memorywith a single non-atomiccp, so a failed sync can leave a partially updated tree that the existing guards do not cover. The two are one causal chain and need one fix.