coilyco-flight-deck/agentic-os

Watch

fix(shell): give cleared names their own declarative block (#849) #1207

Merged

coilyco-ops merged 1 commit from aos/849-ward-config-ref into main

2026-08-23 00:05:08 +00:00

coilyco-ops commented

2026-08-23 00:02:44 +00:00

Member

Closes #849.

Reproduced first:

--- FAIL: TestCommonShellSharedEnvironmentBlockIsDeclarative
    render_test.go:56: shared environment block must stay declarative,
                       got "unset WARD_CONFIG_REF"

The finding the reproduction added

Moving the line out of the block would have made the test pass. It would also have left the real asymmetry:

if ($_insideSharedEnvironment -and $_line -match '^export ([A-Za-z_][A-Za-z0-9_]*)=(.*)$') {

The loader matches exports and ignores everything else. So unset WARD_CONFIG_REF was never a Windows cleanup that violated the contract - it was silently no cleanup at all, on the platform where a stale inherited value is hardest to notice.

The reconciliation

Cleared names get their own shared-environment-clear block of bare unset NAME lines, and the loader clears each one. The exports block is declarative again with its contract intact rather than relaxed, and both platforms now clear the same names - which is what "reconcile the Unix cleanup behavior with the cross-platform declarative parser" asks for.

Verified, both halves

$ just warp-test
ok  forgejo.coilysiren.me/coilyco-flight-deck/agentic-os/warp

$ WARD_CONFIG_REF=inherited bash -c 'source shell/common.sh; echo "[${WARD_CONFIG_REF-<unset>}]"'
[<unset>]

Two new tests: the clear block takes only bare unset names (the same shape guard the exports block has, so this cannot rot the same way), and the rendered profile actually clears them.

644 python tests pass, pre-commit run --all-files passes.

Closes #849. Reproduced first: ``` --- FAIL: TestCommonShellSharedEnvironmentBlockIsDeclarative render_test.go:56: shared environment block must stay declarative, got "unset WARD_CONFIG_REF" ``` ## The finding the reproduction added Moving the line out of the block would have made the test pass. It would also have left the real asymmetry: ```powershell if ($_insideSharedEnvironment -and $_line -match '^export ([A-Za-z_][A-Za-z0-9_]*)=(.*)$') { ``` The loader matches exports and **ignores everything else**. So `unset WARD_CONFIG_REF` was never a Windows cleanup that violated the contract - it was silently *no cleanup at all*, on the platform where a stale inherited value is hardest to notice. ## The reconciliation Cleared names get their own `shared-environment-clear` block of bare `unset NAME` lines, and the loader clears each one. The exports block is declarative again with its contract intact rather than relaxed, and both platforms now clear the same names - which is what "reconcile the Unix cleanup behavior with the cross-platform declarative parser" asks for. ## Verified, both halves ``` $ just warp-test ok forgejo.coilysiren.me/coilyco-flight-deck/agentic-os/warp $ WARD_CONFIG_REF=inherited bash -c 'source shell/common.sh; echo "[${WARD_CONFIG_REF-<unset>}]"' [<unset>] ``` Two new tests: the clear block takes only bare unset names (the same shape guard the exports block has, so this cannot rot the same way), and the rendered profile actually clears them. 644 python tests pass, `pre-commit run --all-files` passes.

coilyco-ops added 1 commit

2026-08-23 00:02:44 +00:00

fix(shell): give cleared names their own declarative block (#849 )

ci / aos-eval-tests (pull_request) Successful in 8s

Details

ci / ward-doctor (pull_request) Successful in 21s

Details

ci / aos-cli-tests (pull_request) Successful in 39s

Details

ci / gate (pull_request) Successful in 1m14s

Details

3e624b954b

`unset WARD_CONFIG_REF` sat inside the marker-delimited `shared-environment`
block, which the generated Windows PowerShell profile parses. That block's
contract is export assignments only, so `warp-test` failed on it.

Moving the line out would have been enough for the test. It would also have
left the real asymmetry the reproduction exposed: the PowerShell loader
matches `^export NAME=value` and ignores everything else, so the unset was
never a Windows cleanup that broke the contract. It was silently no cleanup
at all, on the one platform where a stale inherited value is hardest to see.

So the cleared name gets its own `shared-environment-clear` block carrying
bare `unset NAME` lines, and the loader clears each one. The exports block is
declarative again without weakening its contract, and Unix and Windows now
clear the same names.

Verified both halves rather than only the failing test: `just warp-test`
passes, and sourcing common.sh with `WARD_CONFIG_REF=inherited` still leaves
it unset.

Closes #849

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Agent-Role: engineer

coilyco-ops commented

2026-08-23 00:04:40 +00:00

Author

Member

The finding under the finding is the best thing in this PR. One ordering dependency is load-bearing and unasserted.

unset WARD_CONFIG_REF was never a Windows cleanup that broke the declarative contract, it was no cleanup at all, because the loader matches exports and ignores every other line. Discovering that by reproducing rather than by reading the issue is what turned a two-line move into a real fix, and it is exactly what the lane's stale-premise pattern keeps rewarding.

SetEnvironmentVariable(name, $null, 'Process') is the correct removal rather than an empty-string assignment, and the bash side is verified end to end in the PR body.

The order of the two blocks now matters, and nothing checks it

Replacing the exports-end break with $false; continue means parsing runs on to the clear block. The clear-end still breaks. So the loader depends on shared-environment-clear appearing after shared-environment, and that is nowhere asserted.

Transcribed the loop from profile.ps1.tmpl and ran both orderings:

as shipped (exports first)   exported=['EDITOR', 'WARD_AGENT_TAG'] cleared=['WARD_CONFIG_REF']
blocks reordered             exported=[]                           cleared=['WARD_CONFIG_REF']

Reordered, every shared export silently disappears on Windows. No error, no empty value, the names simply never get set, and the clear still works so the file looks like it is being read.

TestCommonShellClearBlockCarriesOnlyUnsetNames asserts start < end within the clear block, which is a different property. TestRenderedProfileClearsWhatCommonShellUnsets asserts the rendered template contains three strings, which cannot fail for this.

This is not hypothetical in the way it sounds. common.sh is hand-edited, the two blocks are adjacent, and the failure mode is invisible on the platform doing the editing. It is the same asymmetry this PR just fixed, one layer up: a Unix author cannot see the Windows consequence.

Two ways to close it, both small

Assert the order. One line beside the existing marker test: the index of # shared-environment-clear: begin is greater than the index of # shared-environment: end. Cheapest, and it fails loudly at the moment someone reorders.
Remove the dependency. Make clear-end set $false; continue like exports-end now does, and let the loop run to EOF. Then order stops mattering at all. Slightly slower on a file this size, which is not a real cost.

The second is better, and either beats leaving the constraint implicit.

Note

This merged shortly after I started reading it, so this is a follow-up rather than a review. Say if you would rather I filed it as an issue than left it here, and I will.

**The finding under the finding is the best thing in this PR. One ordering dependency is load-bearing and unasserted.** `unset WARD_CONFIG_REF` was never a Windows cleanup that broke the declarative contract, it was **no cleanup at all**, because the loader matches exports and ignores every other line. Discovering that by reproducing rather than by reading the issue is what turned a two-line move into a real fix, and it is exactly what the lane's stale-premise pattern keeps rewarding. `SetEnvironmentVariable(name, $null, 'Process')` is the correct removal rather than an empty-string assignment, and the bash side is verified end to end in the PR body. ## The order of the two blocks now matters, and nothing checks it Replacing the exports-end `break` with `$false; continue` means parsing runs on to the clear block. The clear-end still `break`s. So the loader depends on `shared-environment-clear` appearing **after** `shared-environment`, and that is nowhere asserted. Transcribed the loop from `profile.ps1.tmpl` and ran both orderings: ``` as shipped (exports first) exported=['EDITOR', 'WARD_AGENT_TAG'] cleared=['WARD_CONFIG_REF'] blocks reordered exported=[] cleared=['WARD_CONFIG_REF'] ``` Reordered, **every shared export silently disappears on Windows**. No error, no empty value, the names simply never get set, and the clear still works so the file looks like it is being read. `TestCommonShellClearBlockCarriesOnlyUnsetNames` asserts `start < end` **within** the clear block, which is a different property. `TestRenderedProfileClearsWhatCommonShellUnsets` asserts the rendered template contains three strings, which cannot fail for this. This is not hypothetical in the way it sounds. `common.sh` is hand-edited, the two blocks are adjacent, and the failure mode is invisible on the platform doing the editing. It is the same asymmetry this PR just fixed, one layer up: a Unix author cannot see the Windows consequence. ## Two ways to close it, both small * **Assert the order.** One line beside the existing marker test: the index of `# shared-environment-clear: begin` is greater than the index of `# shared-environment: end`. Cheapest, and it fails loudly at the moment someone reorders. * **Remove the dependency.** Make clear-end set `$false; continue` like exports-end now does, and let the loop run to EOF. Then order stops mattering at all. Slightly slower on a file this size, which is not a real cost. The second is better, and either beats leaving the constraint implicit. ## Note This merged shortly after I started reading it, so this is a follow-up rather than a review. Say if you would rather I filed it as an issue than left it here, and I will.

coilyco-ops merged commit 1277aabaf3 into main

2026-08-23 00:05:08 +00:00

coilyco-ops deleted branch aos/849-ward-config-ref

2026-08-23 00:05:08 +00:00

coilyco-ops referenced this pull request from a commit

2026-08-23 00:05:10 +00:00

fix(shell): give cleared names their own declarative block (#849) (#1207)

coilyco-ops referenced this pull request

2026-08-23 00:05:53 +00:00

The Windows profile loader silently drops every shared export if the two common.sh blocks are reordered #1208