(MOT-4268) feat(harness): add live quickstart canary - #619
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe quickstart validator now tests ChangesQuickstart validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skill-check — worker0 verified, 49 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
harness/tests/quickstart/console_send.py (1)
137-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the traceback for unexpected failures.
except Exceptioncollapses every failure to a one-line message. DeliberateRuntimeError/TimeoutErrormessages read well, but an unexpectedKeyError/AttributeErrorprints e.g.FAIL: 'session_id'with no location — and this stderr is the only diagnostic the shell wrapper points reviewers at.♻️ Proposed tweak
try: asyncio.run(run(parser.parse_args())) + except (RuntimeError, TimeoutError, OSError) as error: + progress(f"FAIL: {error}") + raise SystemExit(1) from error except Exception as error: - progress(f"FAIL: {error}") + progress(f"FAIL: {error!r}\n{traceback.format_exc()}") raise SystemExit(1) from errorThe
json.dumpsstatic-analysis hint on line 95 is a false positive — this is a CLI script, not a web handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/tests/quickstart/console_send.py` around lines 137 - 141, Update the top-level exception handling around asyncio.run in the console script to preserve full tracebacks for unexpected failures while retaining concise FAIL messages for deliberate RuntimeError and TimeoutError cases. Keep the existing exit status behavior and do not change the unrelated json.dumps usage.harness/tests/quickstart/run-ci.sh (2)
47-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the three near-identical numeric validators into a helper.
Three copies of the same 9-line validate-and-normalize block; a small helper keeps future timeouts one-liners.
♻️ Proposed helper
+require_positive_int() { + local name=$1 value=$2 + [[ "$value" =~ ^[0-9]+$ ]] || { + echo "$name must be a positive integer" >&2 + exit 2 + } + value=$((10#$value)) + ((value > 0)) || { + echo "$name must be a positive integer" >&2 + exit 2 + } + printf '%s' "$value" +} + +wait_seconds=$(require_positive_int HARNESS_QUICKSTART_WAIT_SECONDS "$wait_seconds") +add_timeout_seconds=$(require_positive_int HARNESS_QUICKSTART_ADD_TIMEOUT_SECONDS "$add_timeout_seconds") +turn_timeout_seconds=$(require_positive_int HARNESS_QUICKSTART_TURN_TIMEOUT_SECONDS "$turn_timeout_seconds")Note the helper exits from a subshell, so pair it with an explicit status check if you adopt it verbatim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/tests/quickstart/run-ci.sh` around lines 47 - 73, Introduce a shared helper for validating and normalizing positive numeric timeout values, including the variable name in its error message and returning failure rather than terminating from a subshell. Replace the duplicated validation blocks for wait_seconds, add_timeout_seconds, and turn_timeout_seconds with helper calls, each followed by an explicit status check that exits with status 2 on failure.
382-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
websocketsfor the quickstart canary.The canary installs
websocketswithout a version constraint, so it can fail or start producing deprecation warnings if PyPI serves a release that changes the imported API surface. Pin the dependency through an env var/default so the canary result stays tied to a known version.♻️ Proposed pin
- "$run_root/venv/bin/pip" install --quiet --disable-pip-version-check websockets \ + "$run_root/venv/bin/pip" install --quiet --disable-pip-version-check \ + "websockets==${HARNESS_QUICKSTART_WEBSOCKETS_VERSION:-16.0.1}" \ >>"$log_dir/console-send.log" 2>&1 \ || die "pip install websockets failed (see console-send.log)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/tests/quickstart/run-ci.sh` around lines 382 - 384, Update the quickstart canary’s websockets installation in the surrounding run-ci flow to use a configurable version environment variable with a known pinned default, and install that exact requirement instead of the unversioned package. Preserve the existing logging and die-on-failure behavior.harness/tests/quickstart/README.md (1)
24-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the remaining quickstart tunables.
The validator accepts
HARNESS_QUICKSTART_ARTIFACTS_DIR,III_INSTALL_URL,III_CHANNEL,HARNESS_QUICKSTART_WAIT_SECONDS,HARNESS_QUICKSTART_ADD_TIMEOUT_SECONDS,HARNESS_QUICKSTART_TURN_TIMEOUT_SECONDS, andHARNESS_QUICKSTART_PROMPT; this README currently only documentsHARNESS_QUICKSTART_MODELandHARNESS_QUICKSTART_TRACE.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/tests/quickstart/README.md` around lines 24 - 34, Update the quickstart validator documentation near the local execution instructions to document all remaining tunables: HARNESS_QUICKSTART_ARTIFACTS_DIR, III_INSTALL_URL, HARNESS_QUICKSTART_WAIT_SECONDS, HARNESS_QUICKSTART_ADD_TIMEOUT_SECONDS, HARNESS_QUICKSTART_TURN_TIMEOUT_SECONDS, and HARNESS_QUICKSTART_PROMPT, alongside the already documented channel, model, and trace settings..github/workflows/harness-quickstart.yml (1)
67-67: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
persist-credentials: falseon checkout.The job only reads the repo; leaving the token in
.git/configis unnecessary exposure (zizmorartipacked).🔒 Proposed change
- - uses: actions/checkout@v5 + - uses: actions/checkout@v5 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/harness-quickstart.yml at line 67, Update the actions/checkout@v5 step in the workflow to set persist-credentials to false, ensuring the repository read-only job does not retain the checkout token in Git configuration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/harness-quickstart.yml:
- Around line 158-172: Update the jq failure condition in the result-summary
block to apply the alternative operator to failure_reason before comparing it
with an empty string, preventing missing values from rendering as null. Also
expose matrix.channel through an env variable and reference that variable in the
terminal-recording message to resolve the template-injection finding.
- Around line 42-56: Update the Slack post step around the payload and curl
invocation to use the configured Slack channel ID rather than the mutable
channel name, while preserving the existing message content. Remove
--retry-all-errors and retain only bounded retries appropriate for the
non-idempotent chat.postMessage request.
In `@harness/tests/quickstart/quickstart.tape`:
- Around line 23-26: Update the Wait assertion in the quickstart tape to use a
line-anchored match for the bare __HARNESS_QUICKSTART_DONE__ prompt, allowing an
optional trailing space, so it cannot match the typed run-ci.sh command. Keep
the subsequent Sleep unchanged.
In `@harness/tests/quickstart/run-ci.sh`:
- Around line 386-395: Update the console_send.py failure path in run-ci.sh to
redirect stderr directly to console-send.log instead of using asynchronous
process substitution with tee. When the command fails, emit the saved log
contents to stderr before calling die, while preserving the existing failure
message and artifact paths.
---
Nitpick comments:
In @.github/workflows/harness-quickstart.yml:
- Line 67: Update the actions/checkout@v5 step in the workflow to set
persist-credentials to false, ensuring the repository read-only job does not
retain the checkout token in Git configuration.
In `@harness/tests/quickstart/console_send.py`:
- Around line 137-141: Update the top-level exception handling around
asyncio.run in the console script to preserve full tracebacks for unexpected
failures while retaining concise FAIL messages for deliberate RuntimeError and
TimeoutError cases. Keep the existing exit status behavior and do not change the
unrelated json.dumps usage.
In `@harness/tests/quickstart/README.md`:
- Around line 24-34: Update the quickstart validator documentation near the
local execution instructions to document all remaining tunables:
HARNESS_QUICKSTART_ARTIFACTS_DIR, III_INSTALL_URL,
HARNESS_QUICKSTART_WAIT_SECONDS, HARNESS_QUICKSTART_ADD_TIMEOUT_SECONDS,
HARNESS_QUICKSTART_TURN_TIMEOUT_SECONDS, and HARNESS_QUICKSTART_PROMPT,
alongside the already documented channel, model, and trace settings.
In `@harness/tests/quickstart/run-ci.sh`:
- Around line 47-73: Introduce a shared helper for validating and normalizing
positive numeric timeout values, including the variable name in its error
message and returning failure rather than terminating from a subshell. Replace
the duplicated validation blocks for wait_seconds, add_timeout_seconds, and
turn_timeout_seconds with helper calls, each followed by an explicit status
check that exits with status 2 on failure.
- Around line 382-384: Update the quickstart canary’s websockets installation in
the surrounding run-ci flow to use a configurable version environment variable
with a known pinned default, and install that exact requirement instead of the
unversioned package. Preserve the existing logging and die-on-failure behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f086d02-28e9-4449-ae36-ed716594b245
📒 Files selected for processing (5)
.github/workflows/harness-quickstart.ymlharness/tests/quickstart/README.mdharness/tests/quickstart/console_send.pyharness/tests/quickstart/quickstart.tapeharness/tests/quickstart/run-ci.sh
| SLACK_CHANNEL: worker-releases | ||
| III_CHANNEL: ${{ matrix.channel }} | ||
| RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} | ||
| run: | | ||
| set -euo pipefail | ||
| text="⏳ *Harness quickstart* started · \`${III_CHANNEL}\` · <${RUN_URL}|view run>" | ||
| payload=$(jq -n \ | ||
| --arg channel "$SLACK_CHANNEL" \ | ||
| --arg text "$text" \ | ||
| '{channel: $channel, text: $text}') | ||
| response=$(curl -sS --retry 3 --retry-all-errors \ | ||
| -X POST https://slack.com/api/chat.postMessage \ | ||
| -H "Authorization: Bearer ${SLACK_BOT_TOKEN}" \ | ||
| -H "Content-Type: application/json; charset=utf-8" \ | ||
| -d "$payload") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Slack chat.postMessage still support channel name instead of channel ID?
💡 Result:
Yes, the Slack chat.postMessage API method still supports using a channel name instead of a channel ID [1][2]. According to the official Slack API documentation, you can pass either the channel name (e.g., #general) or the encoded channel ID to the channel parameter [1]. However, while this is supported, it is generally discouraged in production environments. Channel names can change over time, whereas channel IDs remain permanent identifiers [3]. Furthermore, most other Slack API methods that require a channel identifier do not support names and specifically require the channel ID [3]. Using the channel ID is considered the best practice to ensure reliability and consistency across your integration [3].
Citations:
- 1: https://docs.slack.dev/reference/methods/chat.postmessage.md
- 2: https://github.com/slackapi/java-slack-sdk/blob/main/slack-api-client/src/main/java/com/slack/api/methods/request/chat/ChatPostMessageRequest.java
- 3: https://stackoverflow.com/questions/75171192/chat-postmessage-with-channel-name-instead-of-channel-id
🌐 Web query:
Slack chat.postMessage channel parameter "channel" accept bare channel name without # Slack API
💡 Result:
Yes, the Slack API chat.postMessage channel parameter accepts a bare channel name (without the # prefix) [1][2][3][4]. According to official Slack API documentation, the channel argument can be an encoded channel ID (e.g., C123456) or the channel's name [1][2]. When passing the name, you do not need to include the hash (#) symbol [1][2]. While the API allows using channel names, it is generally recommended to use the channel's unique ID instead [5]. Channel names can be renamed, whereas channel IDs are persistent, ensuring your application remains reliable if a channel's name is changed [5][6]. You can retrieve the ID associated with a channel name by using the conversations.list API method [1][5][7].
Citations:
- 1: https://docs.slack.dev/reference/methods/chat.postmessage.md
- 2: https://github.com/slackhq/slack-api-docs/blob/master/methods/chat.postMessage.md
- 3: https://github.com/slackapi/java-slack-sdk/blob/main/slack-api-client/src/main/java/com/slack/api/methods/request/chat/ChatPostMessageRequest.java
- 4: https://www.postman.com/slackhq/slack-api/request/3iqk2yg/chat-post-message
- 5: https://stackoverflow.com/questions/69308538/lookup-channel-by-name
- 6: https://docs.slack.dev/reference/methods/conversations.create.md
- 7: https://docs.slack.dev/reference/methods/conversations.list.md
🌐 Web query:
Slack API channel names chat.postMessage channel parameter no # examples
💡 Result:
The Slack API chat.postMessage method's channel parameter accepts either an encoded channel ID (e.g., C12345678) or a channel name [1][2]. When using a channel name, you do not need to include the hash (#) prefix [2]. You can pass the name as a plain string (e.g., "general") [2]. While the API documentation has historically permitted channel names [2], using the channel ID is strongly recommended because channel names are not globally unique across workspaces and can be changed by users, whereas the channel ID is immutable [1]. To obtain the correct channel ID, you can use the conversations.list API method [1][3].
Citations:
- 1: https://docs.slack.dev/reference/methods/chat.postmessage.md
- 2: https://github.com/slackhq/slack-api-docs/blob/master/methods/chat.postMessage.md
- 3: Find channel by name without list method slackapi/node-slack-sdk#1543
🌐 Web query:
Slack API channel_not_name channel_param_is_not_valid chat.postMessage channel name
💡 Result:
The channel_not_found error in the Slack API's chat.postMessage method indicates that the Slack platform cannot locate the specified channel or that your application does not have the necessary permission to post into it [1][2][3]. Common causes and solutions include: 1. App Membership: For private channels, your Slack app or bot must be explicitly added to the channel as a member [2][4]. You can invite your bot by typing /invite @YourBotName in the channel [3][4]. 2. Incorrect Identifier: Ensure the value passed for the channel argument is correct [5]. While Slack often supports both channel names (e.g., #general) and channel IDs (e.g., C024BE91L), using the unique channel ID is more reliable and recommended to avoid ambiguity [6][5][4]. 3. Permission Scopes: Verify that your application has the required scopes for the requested action, such as chat:write or chat:write.public (for public channels where the bot is not a member) [7][4]. 4. Token Context: Ensure you are using the correct token associated with the intended workspace. Errors can occur if an OAuth token is used for a workspace where the app was not installed, or if there is a mismatch between the token and the channel access [8][5]. 5. Formatting: If making manual API requests, ensure parameters are correctly encoded in the request body (e.g., application/x-www-form-urlencoded) rather than incorrectly placed in headers or improperly formatted [8][9][10].
Citations:
- 1: https://docs.slack.dev/reference/methods/chat.postmessage.md
- 2: https://stackoverflow.com/questions/61468020/slack-web-api-returning-channel-not-found-chat-postmessage-to-a-private-channel
- 3: Issue with running python-slack-sdk using test application. slackapi/python-slack-sdk#1316
- 4: https://knock.app/blog/troubleshooting-channel-not-found-in-slack-incoming-webhooks
- 5: https://help.zapier.com/hc/en-us/articles/31661561373837-Slack-error-channel-not-found
- 6: https://github.com/slackhq/slack-api-docs/blob/master/methods/chat.postMessage.md
- 7: https://stackoverflow.com/questions/69155093/channel-not-found-authed-user-cannot-post-a-message-to-a-channel-via-slack-api
- 8: https://stackoverflow.com/questions/52707096/slack-via-oauth-chat-postmessage-error-channel-not-found-even-though-chann
- 9: https://www.postman.com/slackhq/slack-api/request/3iqk2yg/chat-post-message
- 10: https://raw.githubusercontent.com/api-evangelist/slack/refs/heads/main/openapi/slack-chat-openapi.yml
🌐 Web query:
Slack chat.postMessage channel parameter "channel" accepts channel name or encoded channel ID
💡 Result:
The channel parameter in the Slack chat.postMessage method accepts either a Slack-encoded channel ID (e.g., C123456) or a channel name (e.g., #general) [1][2][3]. While it is possible to use a channel name, it is generally recommended to use the channel ID [4]. Channel names are mutable and can be changed by workspace members, whereas channel IDs are persistent and unique identifiers [4]. Furthermore, most other Slack API methods that require a channel reference only accept the channel ID, making the use of IDs a more consistent practice across your integration [4]. If you use a channel name, the API will resolve it to the corresponding channel, but relying on IDs ensures greater stability for your application [1][4].
Citations:
- 1: https://docs.slack.dev/reference/methods/chat.postmessage.md
- 2: https://github.com/slackhq/slack-api-docs/blob/master/methods/chat.postMessage.md
- 3: https://www.postman.com/slackhq/slack-api/request/3iqk2yg/chat-post-message
- 4: https://stackoverflow.com/questions/75171192/chat-postmessage-with-channel-name-instead-of-channel-id
🌐 Web query:
curl --retry-all-errors retry POST request duplicate submission non-idempotent documentation
💡 Result:
Using the --retry-all-errors option in curl is considered a "sledgehammer" approach to retrying transfers and should be used with extreme caution, particularly with non-idempotent HTTP methods like POST [1][2]. Key Considerations: 1. Risk of Duplicate Submissions: When you use --retry-all-errors, curl will attempt to retry the request for any error it encounters [3][4]. If the initial request was a POST or another non-idempotent method that partially reached the server—or was processed before a connection error occurred—the retry will trigger a duplicate submission on the server side [1][2][5]. 2. Scope of Option: This option must be used in conjunction with --retry [1][4]. By default, curl only retries on certain connection errors and transient HTTP 5xx server errors [5]. Enabling --retry-all-errors forces retries on nearly all failures, including many 4xx client errors that would otherwise be considered final [1][5]. 3. Data Handling: The documentation warns against using this option with redirected input or output (e.g., using pipes | or >), as curl cannot automatically "reset" these streams before a retry, which can lead to receiving or sending duplicate data [1]. 4. Best Practices: * Avoid --retry-all-errors for any request that is not explicitly safe to repeat [2][5]. * Prioritize idempotent methods (like GET or HEAD) when configuring automatic retry logic [2][5]. * For POST/PUT requests, handle retries at the application level where you can verify the state of the previous submission before attempting another [2][5]. * If you must use it, ensure the command is part of a controlled, well-understood environment rather than a default configuration [1][2]. Official documentation emphasizes that you are generally better off handling specific, expected failure scenarios within a script rather than relying on this broad, indiscriminate retry mechanism [1].
Citations:
- 1: https://fuchsia.googlesource.com/third_party/curl/+/main/docs/cmdline-opts/retry-all-errors.md
- 2: https://www.simplified.guide/curl/retry-configure-transient-error
- 3: https://everything.curl.dev/usingcurl/downloads/retry.html
- 4: https://daniel.haxx.se/blog/2020/06/24/curl-7-71-0-blobs-and-retries/
- 5: https://www.developnsolve.com/post/curl-command-for-retry-logic
Use the Slack channel ID and limit retries for non-idempotent Slack posts.
chat.postMessage accepts channel names, but names are mutable and can fail with channel_not_found; channel_id is preferred for reliability. Also, --retry-all-errors will retry the POST submission more broadly than --retry 3, which risks duplicate Slack posts if the first request was processed before a failure was reported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/harness-quickstart.yml around lines 42 - 56, Update the
Slack post step around the payload and curl invocation to use the configured
Slack channel ID rather than the mutable channel name, while preserving the
existing message content. Remove --retry-all-errors and retain only bounded
retries appropriate for the non-idempotent chat.postMessage request.
| jq -r ' | ||
| "| Status | CLI | Duration |", | ||
| "| --- | --- | ---: |", | ||
| "| \(.status) | \(.cli_version) | \(.elapsed_ms) ms |", | ||
| (if .failure_reason != "" then "", "Failure: \(.failure_reason)" else empty end) | ||
| "| Status | Channel | CLI | GLM canary | Duration |", | ||
| "| --- | --- | --- | --- | ---: |", | ||
| "| \(.status) | \(.channel) | \(.cli_version) | \(.message_check) | \(.elapsed_ms) ms |", | ||
| (if .failure_reason != "" then "", "**Failure:** \(.failure_reason)" else empty end) | ||
| ' target/harness-quickstart/result.json | ||
| else | ||
| echo "### Harness quickstart" | ||
| echo | ||
| echo "No quickstart result was produced." | ||
| fi | ||
| if [[ -f target/harness-quickstart/quickstart.mp4 ]]; then | ||
| echo | ||
| echo "Terminal recording: \`quickstart.mp4\` in the \`harness-quickstart-${{ matrix.channel }}\` artifact." | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
failure_reason renders as null when the key is absent.
.failure_reason != "" is true for null, so a missing key emits **Failure:** null in the job summary. Use the alternative operator first.
Separately, moving ${{ matrix.channel }} (Line 171) into an env: var silences the zizmor template-injection finding; the value itself is safe here since it comes from an enumerated choice input.
🐛 Proposed fix
- (if .failure_reason != "" then "", "**Failure:** \(.failure_reason)" else empty end)
+ (if (.failure_reason // "") != "" then "", "**Failure:** \(.failure_reason)" else empty end)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jq -r ' | |
| "| Status | CLI | Duration |", | |
| "| --- | --- | ---: |", | |
| "| \(.status) | \(.cli_version) | \(.elapsed_ms) ms |", | |
| (if .failure_reason != "" then "", "Failure: \(.failure_reason)" else empty end) | |
| "| Status | Channel | CLI | GLM canary | Duration |", | |
| "| --- | --- | --- | --- | ---: |", | |
| "| \(.status) | \(.channel) | \(.cli_version) | \(.message_check) | \(.elapsed_ms) ms |", | |
| (if .failure_reason != "" then "", "**Failure:** \(.failure_reason)" else empty end) | |
| ' target/harness-quickstart/result.json | |
| else | |
| echo "### Harness quickstart" | |
| echo | |
| echo "No quickstart result was produced." | |
| fi | |
| if [[ -f target/harness-quickstart/quickstart.mp4 ]]; then | |
| echo | |
| echo "Terminal recording: \`quickstart.mp4\` in the \`harness-quickstart-${{ matrix.channel }}\` artifact." | |
| fi | |
| jq -r ' | |
| "| Status | Channel | CLI | GLM canary | Duration |", | |
| "| --- | --- | --- | --- | ---: |", | |
| "| \(.status) | \(.channel) | \(.cli_version) | \(.message_check) | \(.elapsed_ms) ms |", | |
| (if (.failure_reason // "") != "" then "", "**Failure:** \(.failure_reason)" else empty end) | |
| ' target/harness-quickstart/result.json |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 171-171: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/harness-quickstart.yml around lines 158 - 172, Update the
jq failure condition in the result-summary block to apply the alternative
operator to failure_reason before comparing it with an empty string, preventing
missing values from rendering as null. Also expose matrix.channel through an env
variable and reference that variable in the terminal-recording message to
resolve the template-injection finding.
Source: Linters/SAST tools
| Type "harness/tests/quickstart/run-ci.sh" | ||
| Enter | ||
| Wait /__HARNESS_QUICKSTART_DONE__>/ | ||
| Sleep 2s |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
VHS tape Wait command Line scope regex anchoring semantics
💡 Result:
In the VHS (Virtual Terminal Recording) tool, the Wait command is used to pause execution until a specific regular expression matches the terminal's output [1][2]. Scope Semantics: The command supports two scopes, defined by the optional modifiers +Line and +Screen [1][3]. The scope determines the source of the text against which the regular expression is matched [4]. - Line: The default scope [1]. It monitors only the current line of the terminal (as determined by the cursor's position) [4][5]. - Screen: Monitors the entire terminal buffer [4]. When used, VHS joins all lines in the buffer with newline characters and performs the regex match against the resulting string [4]. Regex Anchoring Semantics: The regular expression provided to the Wait command is used with the Go standard library's regexp.MatchString function [4]. This function does not implicitly anchor the expression to the start or end of the string [4]. Therefore, the provided regex will match if the pattern exists anywhere within the scope (the current line or the entire screen buffer, depending on the chosen scope) [5]. Additional Details: - Default Pattern: If no regex is provided, the command defaults to matching the pattern />$/ [1][6]. - Timeout: The default timeout is 15 seconds [1]. This can be adjusted using the WaitTimeout setting or the @ duration modifier in the command (e.g., Wait@10ms) [1][4]. - Regex Delimiters: The pattern must be enclosed in forward slashes (e.g., /regex/) [1][3]. Historically, this has made escaping literal forward slashes within the pattern difficult, as the parser treats them as terminators; users often use hex escape sequences like \x{2F} to represent a literal forward slash [7].
Citations:
- 1: https://github.com/charmbracelet/vhs/blob/main/README.md
- 2: https://deepwiki.com/charmbracelet/vhs/3.1-command-execution
- 3: https://www.mankier.com/1/vhs
- 4: https://github.com/charmbracelet/vhs/blob/f28239f3/command.go
- 5: feat: add Wait to wait for expected output charmbracelet/vhs#257
- 6: https://github.com/charmbracelet/vhs/
- 7: Impossible to escape / in Wait regex charmbracelet/vhs#592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate tape and workflow =="
fd -a 'quickstart\.tape|.*\.(ya?ml)$' . | sed 's#^\./##' | head -200
echo "== tape content =="
cat -n harness/tests/quickstart/quickstart.tape
echo "== relevant workflow files =="
for f in $(fd '.*\.(ya?ml)$' .github workflows . | head -50); do
if rg -q 'quickstart|run-ci|validate|ffprobe|__HARNESS_QUICKSTART_DONE__' "$f" 2>/dev/null; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: iii-hq/workers
Length of output: 25506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning quickstart/run-ci/ffprobe/slack upload =="
rg -n --hidden --glob '!pnpm-lock.yaml' --glob '!*.yaml' --glob '!package-lock.json' --glob '!yarn.lock' \
'quickstart|run-ci|__HARNESS_QUICKSTART_DONE__|ffprobe|upload|slack' . 2>/dev/null | head -300
echo "== quickstart runner =="
if [ -f harness/tests/quickstart/run-ci.sh ]; then
cat -n harness/tests/quickstart/run-ci.sh
fi
echo "== nearby harness quickstart files =="
find harness/tests/quickstart -maxdepth 2 -type f -print | sort | while read -r f; do
echo "--- $f ---"
sed -n '1,220p' "$f"
done
echo "== workflow files =="
find .github harness -type d -maxdepth 4 2>/dev/null | sed 's#^\./##' | sort | head -80
find .github harness -type f -regex '.*\.\(ya?ml\)$' -print 2>/dev/null | while read -r f; do
if rg -q 'quickstart|run-ci|__HARNESS_QUICKSTART_DONE__|ffprobe|slack|upload' "$f" 2>/dev/null; then
echo "--- $f ---"
sed -n '1,260p' "$f"
fi
doneRepository: iii-hq/workers
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact workflow path candidates =="
find . -type f \( -path '*/.github/workflows/*' -o -path '*/harness/tests/quickstart/*' \) -print 2>/dev/null | sort
echo "== workflow quickstart references concise =="
rg -n --hidden --glob '!.git/*' --glob '!pnpm-lock.yaml' --glob '!package-lock.json' --glob '!yarn.lock' \
'harness/tests/quickstart|quickstart-validate|quickstart\.tape|HARNESS_QUICKSTART|ffprobe|upload-artifact|slack|Notify|notifications|status' .github harness 2>/dev/null | head -300
echo "== exact workflow relevant sections candidate =="
for f in .github/workflows/_harness-e2e.yml .github/workflows/quickstart.yml harness/Makefile harness/tests/quickstart/run-ci.sh; do
[ -f "$f" ] || continue
echo "--- $f ---"
rg -n -C 4 'quickstart-validate|quickstart\.tape|ffmpeg|ffprobe|upload-artifact|slack|Notify|notifications|status|HARNESS_QUICKSTART' "$f"
doneRepository: iii-hq/workers
Length of output: 41956
Anchor the VHS prompt match so Wait doesn’t match the typed command.
Wait /__HARNESS_QUICKSTART_DONE__>/ is a line-scoped substring match, and __HARNESS_QUICKSTART_DONE__> harness/tests/quickstart/run-ci.sh appears on the final line right after Enter. Anchoring to the bare prompt, e.g. Wait+Line /^__HARNESS_QUICKSTART_DONE__> ?$/, prevents the recording from stopping before the validator finishes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/tests/quickstart/quickstart.tape` around lines 23 - 26, Update the
Wait assertion in the quickstart tape to use a line-anchored match for the bare
__HARNESS_QUICKSTART_DONE__ prompt, allowing an optional trailing space, so it
cannot match the typed run-ci.sh command. Keep the subsequent Sleep unchanged.
| if ! "$run_root/venv/bin/python" "$script_dir/console_send.py" \ | ||
| --url "ws://127.0.0.1:$console_port/ws" \ | ||
| --model "$send_model" --provider "$send_provider" \ | ||
| --prompt "$send_prompt" --timeout "$turn_timeout_seconds" \ | ||
| --model "$send_model" \ | ||
| --provider "$send_provider" \ | ||
| --prompt "$send_prompt" \ | ||
| --timeout "$turn_timeout_seconds" \ | ||
| >"$artifact_dir/console-send.json" \ | ||
| 2> >(tee -a "$log_dir/console-send.log" >&2); then | ||
| die "console message send failed (see console-send.log)" | ||
| die "GLM canary failed (see console-send.log)" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Async process substitution can drop the diagnostics die points at.
2> >(tee -a "$log_dir/console-send.log" >&2) is not waited on, so when the canary fails the shell can reach die (and then the EXIT trap) before tee flushes — leaving "see console-send.log" pointing at a truncated file. Redirect straight to the log and echo it back instead.
🐛 Proposed fix
if ! "$run_root/venv/bin/python" "$script_dir/console_send.py" \
--url "ws://127.0.0.1:$console_port/ws" \
--model "$send_model" \
--provider "$send_provider" \
--prompt "$send_prompt" \
--timeout "$turn_timeout_seconds" \
>"$artifact_dir/console-send.json" \
- 2> >(tee -a "$log_dir/console-send.log" >&2); then
+ 2>>"$log_dir/console-send.log"; then
+ tail -n 40 "$log_dir/console-send.log" >&2 || true
die "GLM canary failed (see console-send.log)"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! "$run_root/venv/bin/python" "$script_dir/console_send.py" \ | |
| --url "ws://127.0.0.1:$console_port/ws" \ | |
| --model "$send_model" --provider "$send_provider" \ | |
| --prompt "$send_prompt" --timeout "$turn_timeout_seconds" \ | |
| --model "$send_model" \ | |
| --provider "$send_provider" \ | |
| --prompt "$send_prompt" \ | |
| --timeout "$turn_timeout_seconds" \ | |
| >"$artifact_dir/console-send.json" \ | |
| 2> >(tee -a "$log_dir/console-send.log" >&2); then | |
| die "console message send failed (see console-send.log)" | |
| die "GLM canary failed (see console-send.log)" | |
| fi | |
| if ! "$run_root/venv/bin/python" "$script_dir/console_send.py" \ | |
| --url "ws://127.0.0.1:$console_port/ws" \ | |
| --model "$send_model" \ | |
| --provider "$send_provider" \ | |
| --prompt "$send_prompt" \ | |
| --timeout "$turn_timeout_seconds" \ | |
| >"$artifact_dir/console-send.json" \ | |
| 2>>"$log_dir/console-send.log"; then | |
| tail -n 40 "$log_dir/console-send.log" >&2 || true | |
| die "GLM canary failed (see console-send.log)" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/tests/quickstart/run-ci.sh` around lines 386 - 395, Update the
console_send.py failure path in run-ci.sh to redirect stderr directly to
console-send.log instead of using asynchronous process substitution with tee.
When the command fails, emit the saved log contents to stderr before calling
die, while preserving the existing failure message and artifact paths.
1 participant
Summary
#worker-releases, update its final status, and publish diagnostics in its threadWhy
The deterministic E2E suite runs repository binaries, so it does not prove that published registry artifacts can install and complete a real Harness request. This adds that production-path canary while keeping artifacts and notifications concise.
Validation
zai/glm-5.2(message_check: passed, 52 seconds)bash -n harness/tests/quickstart/run-ci.shpython3 -m py_compile harness/tests/quickstart/console_send.pygit diff --checkZAI_API_KEYwas not persisted in artifactsFixes MOT-4268
Summary by CodeRabbit
New Features
latestandnextinstaller channels.Bug Fixes
Documentation