fix: separate shell protocol from diagnostics - #23
Merged
Conversation
Closes #21 Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f92f1f5b5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
`[[:cntrl:]]` is resolved by the shell against the locale the release inherited, so it described the host rather than the contract. Measured across the shells a release can be started by: dash, and bash under `LC_ALL=C`, match C0 and DEL; bash in a UTF-8 locale also matches the C1 block, and glibc's tables put U+2028 and U+2029 in the class too. Both directions of error followed. In `bin/castle` the class could refuse a version the contract permits - every valid UTF-8 codepoint outside C0, DEL and C1 - even though the decoder behind it accepts one, so the managed set depended on the locale. In the `env.sh` fragment the same class under-matched: a C1-bearing marker was refused as unusable under bash in a UTF-8 locale and carried under dash, which is why `env_script_test.exs:1061` passed on macOS and failed on every Ubuntu cell. Use a literal set of the C0 bytes and DEL, built once with `printf`, in all three places. It is identical under dash, bash 3.2 and bash 5.3 in POSIX mode, under both a UTF-8 locale and `LC_ALL=C`. In `bin/castle` it is now strictly a shortcut in front of the decoder - restricted to bytes the decoder also forbids, so it can never refuse what the decoder would accept. The fragment therefore refuses C0 and DEL byte-exactly and says nothing about C1, which is a real limit rather than a hole: a POSIX shell has no portable way to match the C1 block, since its UTF-8 encoding is two bytes that bash in a UTF-8 locale sees as one character and dash sees as two - a bracket of the composed characters would refuse every ordinary version carrying U+00A0 to U+00BF under dash. Nothing selects a release on that check alone, and the tests now pin the two guards that carry it: a C1-bearing pair that agrees is still refused for the version directory, and a version outside ASCII is still selected. Claude-Session: https://claude.ai/code/session_01RotroiBdbidiCRX3KqE3Dy Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ll-protocol # Conflicts: # priv/env.sh.eex # test/forecastle/env_script_test.exs
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
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.
Summary
Stack
This PR is stacked on #22 because both changes touch the generated launcher and environment hooks. Review only the single commit above
issue/20-diagnostics; after #22 lands, this PR can targetrelease/1.0.0directly.Validation
mix precommit/bin/sh -non both generated templatesReview
Claude Code adversarial review ran for the configured five-round maximum. All branch-specific findings were addressed. The final pass found four additional items; measured-launcher coverage, lazy marker display formatting, an explicit
:okcommit contract, and accurate invocation-tag wording landed after the cap, so no sixth review was run.Two valid findings were deferred because their implementations are byte-for-byte unchanged on the #22 base and outside issue #21: the heart guard treats an
awkexecution error as no-heart, and the install signal trap arms its outcome after the RPC returns. Neither was changed in this branch.Closes #21