fix(cli): preserve result metadata routing (#358 #401) - #20
Conversation
![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: 824d59882c
ℹ️ 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".
| if action is None and option.startswith("-") and not option.startswith("--"): | ||
| action = actions.get(option[:2]) | ||
| attached = action is not None and len(option) > 2 |
There was a problem hiding this comment.
When a parse error follows a valid combined short form such as hermes chat -Qqhello --result-meta-fd FD --max-turns nope, argparse recognizes -Qqhello as -Q plus -q hello, but this scanner treats the entire token as the first -Q action and never records the query. _eligible_result_metadata_parse_failure_argv() therefore returns false, causing main() to close the descriptor without the promised unknown-failure frame; parse short-option clusters with argparse-equivalent semantics before deciding eligibility.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
824d598 to
53087da
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53087da251
ℹ️ 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".
| action = actions.get(option) | ||
| attached = "=" in token | ||
| attached_value = token.split("=", 1)[1] if attached else None | ||
| if action is None and option.startswith("--"): |
There was a problem hiding this comment.
When argparse accepts a unique abbreviation such as hermes chat --query hello --result-meta-f=FD --max-turns nope, this branch recognizes the abbreviated option, but _early_claim_result_metadata_argv() only looks for the exact --result-meta-fd spelling. Consequently no owner exists in the parse-error handler, so the descriptor receives no terminal failure frame despite the invocation selecting result metadata; apply the same abbreviation semantics during the early claim and duplicate check.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| if action is None: | ||
| if token.startswith("-") and not query_values: | ||
| return False | ||
| index += 1 |
There was a problem hiding this comment.
For an invalid invocation such as hermes chat -q hello -- --result-meta-fd FD, argparse treats everything after -- as positional text and rejects it, but the early scan still claims FD and this loop continues past the terminator because a query was already found. The new parse-failure path then writes metadata to that unrelated pipe and converts the usage error to exit 0; return false when the chat-side scanner encounters --.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
53087da to
6bf83f8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bf83f8f0e
ℹ️ 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".
| owner = getattr(cli, "result_meta_fd", None) | ||
| if owner is None or owner.closed: | ||
| return False | ||
| cli._publish_abnormal_result_metadata(interrupted=False) |
There was a problem hiding this comment.
When KeyboardInterrupt occurs after the descriptor is claimed but before the narrower query handlers run—for example during _prepare_agent_startup() or _init_agent()—the new broad BaseException fallback reaches this call and publishes failed=true, interrupted=false, and failure_class="unknown_failure". This violates the metadata contract that distinguishes user cancellation from unexpected failure and can cause automation to retry or report a cancelled run as an error; retain the caught exception and pass interrupted=True for KeyboardInterrupt in both this direct-API path and the sibling startup fallback in hermes_cli/main.py.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
hermes-agent/hermes_cli/main.py
Lines 13186 to 13187 in dcdcdb4
Guard the descriptor throughout pre-parser startup
For a valid metadata invocation, ownership is acquired here before quarantine cleanup, interrupted-install recovery, and the full parser/subcommand construction, but no surrounding guard handles the owner until parsing begins. At the exact head, a KeyboardInterrupt during recovery or a BaseException from one of the subsequent parser-building imports therefore exits without publishing interrupted/failure metadata; this is a fresh gap outside the newly added _prepare_agent_startup() fallback, so ownership cleanup/publication needs to cover the entire interval after this claim.
AGENTS.md reference: AGENTS.md:L54-L57
ℹ️ 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".
| if index + 1 >= len(argv) or argv[index + 1].startswith("-"): | ||
| return False | ||
| query_values.append(argv[index + 1]) | ||
| index += 2 |
There was a problem hiding this comment.
When an invocation passes an empty query as a separate argument, such as hermes chat -q '' --result-meta-fd FD --max-turns nope, argparse accepts the empty string but _validate_result_metadata_args() would reject it as no query. This scanner instead appends '', treats the invocation as eligible, publishes an unknown-failure frame, and changes the usage error from exit 2 to exit 0; apply the same nonempty-query check used for the attached-value branch.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea21372a09
ℹ️ 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".
| if len(query_values) != 1: | ||
| return False |
There was a problem hiding this comment.
When a parse error follows an otherwise valid invocation with repeated query options, such as hermes chat -q first -q second --result-meta-fd FD --max-turns nope, argparse permits both query flags and would use the last value, but this exact-one check declares the already-claimed owner ineligible. The descriptor is therefore closed without the promised unknown-failure frame; mirror argparse's last-value-wins behavior while still rejecting an empty effective query.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
1 participant
Summary
--cl--result-meta-f=FDbefore parse-failure publication--terminatorExact publication gate
4a440e09c09987945430d8c77d2e027c3dec723b6bf83f8f0e536a8fd0b8a0aee2ff675ca46a675bed643ab6a0d6f093f306e5b426b6d67e039facf75f274c8da5e5355161766e9ea6b14baa9c034cc8f2b888c4a38f73e3803f088bVerification
scripts/run_tests.sh tests/hermes_cli/test_result_metadata.py tests/hermes_cli/test_result_metadata_cli.py: 64 passed/home/michal/wc-infrastructure:PYTHONDONTWRITEBYTECODE=1 python3 -m unittest tests.test_hermes_runtime_receipt -q: 28 passeduvx ruff check hermes_cli/main.py tests/hermes_cli/test_result_metadata_cli.py: passedpython3 -m py_compile hermes_cli/main.py tests/hermes_cli/test_result_metadata_cli.py: passedgit diff --check: passedExact-head required checks and fresh exact-head Codex review remain mandatory before merge. Live activation/default canary/model A-B/profile rollout remain separate and were not performed. Issues cermm/wc-infrastructure#358, NousResearch#359, and NousResearch#401 must not be closed by this PR.