Skip to content

Complete Awsh Bash adapter prototype - #8

Open
omry wants to merge 1 commit into
pr6from
pr7
Open

Complete Awsh Bash adapter prototype#8
omry wants to merge 1 commit into
pr6from
pr7

Conversation

@omry

@omry omry commented Aug 14, 2026

Copy link
Copy Markdown
Owner

Add a private coprocess broker for the Awsh control descriptors so ordinary operation children do not inherit the Envoy-facing channel. Support action gates, cooperative cancellation, fail-closed shutdown, and partial results when an operation exits or replaces Bash.

Expand the demo, design notes, and conformance tests for gates, background jobs, PTY resize, curses, nested Bash, descriptor isolation, cancellation, and lifecycle failures.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@omry

omry commented Aug 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review 95d8c74

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 95d8c74745

ℹ️ 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".

Comment thread docs/future/prototype/awsh/awsh-driver.bash Outdated
@omry

omry commented Aug 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review 5e01448

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e014480c8

ℹ️ 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".

Comment thread docs/future/prototype/awsh/README.md
@omry

omry commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Exact head: 301ed74

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 301ed74b6e

ℹ️ 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".

Add a private coprocess broker for the Awsh control descriptors so ordinary operation children do not inherit the Envoy-facing channel. Support action gates, cooperative cancellation, fail-closed shutdown, and partial results when an operation exits or replaces Bash.

Expand the demo, design notes, and conformance tests for gates, background jobs, PTY resize, curses, nested Bash, descriptor isolation, cancellation, and lifecycle failures.
@gitar-bot

gitar-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Completes the Awsh Bash adapter prototype with a private coprocess broker, expanded conformance tests, and robust lifecycle controls. Consider updating the exec statement to use the AWSH_SOURCE_FD constant instead of the hardcoded descriptor 22.

💡 Quality: AWSH_SOURCE_FD constant defined but hardcoded 22 used in exec

📄 docs/future/prototype/awsh/awsh-driver.bash:10 📄 docs/future/prototype/awsh/awsh-driver.bash:301 📄 docs/future/prototype/awsh/awsh-driver.bash:303 📄 docs/future/prototype/awsh/awsh-driver.bash:305

readonly AWSH_SOURCE_FD=22 is introduced and used in the argument validation, but awsh_run_operation hardcodes the descriptor number 22 in exec 22< <(...), source /dev/fd/22, and exec 22<&-. Changing the constant later would silently desync the validation from the actual descriptor the driver opens, reintroducing the collision the validation is meant to prevent. Reference the constant instead, e.g. exec {AWSH_SOURCE_FD}<... won't work with a fixed fd, but eval "exec ${AWSH_SOURCE_FD}< <(printf '%s' "\$1")" or a local local fd=$AWSH_SOURCE_FD; exec ... /dev/fd/$fd keeps a single source of truth.

🤖 Prompt for agents
Code Review: Completes the Awsh Bash adapter prototype with a private coprocess broker, expanded conformance tests, and robust lifecycle controls. Consider updating the `exec` statement to use the `AWSH_SOURCE_FD` constant instead of the hardcoded descriptor `22`.

1. 💡 Quality: AWSH_SOURCE_FD constant defined but hardcoded 22 used in exec
   Files: docs/future/prototype/awsh/awsh-driver.bash:10, docs/future/prototype/awsh/awsh-driver.bash:301, docs/future/prototype/awsh/awsh-driver.bash:303, docs/future/prototype/awsh/awsh-driver.bash:305

   `readonly AWSH_SOURCE_FD=22` is introduced and used in the argument validation, but `awsh_run_operation` hardcodes the descriptor number `22` in `exec 22< <(...)`, `source /dev/fd/22`, and `exec 22<&-`. Changing the constant later would silently desync the validation from the actual descriptor the driver opens, reintroducing the collision the validation is meant to prevent. Reference the constant instead, e.g. `exec {AWSH_SOURCE_FD}<...` won't work with a fixed fd, but `eval "exec ${AWSH_SOURCE_FD}< <(printf '%s' "\$1")"` or a local `local fd=$AWSH_SOURCE_FD; exec ... /dev/fd/$fd` keeps a single source of truth.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants