MT-703 Replace custom env: prefix by conventional $#1
Merged
frantic-openai merged 2 commits intoFeb 26, 2026
Merged
Conversation
frantic-openai
deleted the
frantic/mt-703-replace-custom-env-prefix-by-conventional
branch
ppatel-oai
added a commit
that referenced
this pull request
Mar 17, 2026
chore(elixir): add ppatel CPU optimization workflow
guangyucoder
referenced
this pull request
in guangyucoder/symphony
Apr 13, 2026
Three independent reviewers (conservative + aggressive + codex) on the
prior commits found correctness gaps that would have shipped a
"verify drift detection" feature that doesn't actually catch drift
on real Elixir errors, escalation paths that silently swallow caps,
and a one-way trap door for tickets unlucky enough to trigger drift.
## CRITICAL fixes
1. **Drift trap door** — `verify_drift_escalated` was sticky forever.
`IssueExec.reset_for_rework/1` has zero production callers; the
inline Rework-entry block in `agent_runner.ex` writes its own
partial reset that did NOT include the new fields. So once drift
fired, the ticket could never re-verify even after a human pushed
it back through Rework — `verify_rule` (and now `merge_verify_rule`)
would short-circuit forever, falling to `:no_matching_rule` and
re-escalating each tick with a misleading "check workspace git
state" message.
Fix: add `verify_drift_escalated`, `verify_last_error_sig`,
`merge_needs_verify` to BOTH the Rework-entry reset AND
`escalate_to_human/3`.
2. **Cap exhaustion silently swallowed** — `do_merge_with_ci_check`'s
`handle_merge_conflict` and `handle_mergeability_unknown` returned
`{:error, "...escalating"}`, but `execute_merge_programmatic` just
logged and returned `:ok`. The "2-cycle merge-sync cap" and "10-
unknown cap" advertised in the design doc were unenforceable —
every poll re-attempted merge, hit the cap, swallowed the error.
Fix: route `{:error, reason}` to `escalate_to_human/3` directly.
## HIGH fixes
3. **Drift in Merging infinite-looped via `merge_verify_rule`**.
That rule had no `verify_drift_escalated` guard (only `verify_rule`
did), and the closeout drift branch didn't clear `merge_needs_verify`.
Fix: add drift guard to `merge_verify_rule`; clear `merge_needs_verify`
AND `verify_fix_count` in the drift escalation branch.
4. **`ci_parity_script` shell-injection / quoting**. Path was
interpolated into `sh -lc "./#{script_rel}"` unquoted — a value
like `scripts/x.sh; rm -rf .` would shell-execute. WORKFLOW.md is
operator-controlled but a malicious PR could plant.
Fix: refuse paths matching anything other than `[A-Za-z0-9_./-]+`.
5. **`verify_error_signature` missed real Elixir noise**. Stripped
only timestamps, hex addrs, line:col. Two identical asserts with
different `#PID<...>`, `#Reference<...>`, `/tmp/foo_12345`, test
durations, UUIDs, or non-Z timezone offsets would hash differently
— drift would silently never fire on the exact workloads (flaky
env-dependent tests) it was designed for.
Fix: extend regex set to cover all of the above.
6. **Merging-entry cleanup wiped legitimate post-sync verify units**.
After merge-sync sets `merge_needs_verify=true`, a `verify` unit
in Merging is required state. Cleanup treated it as stale and reset
the flag too — restart bypassed the post-conflict re-verification
entirely.
Fix: extend the cleanup-exemption to include `kind == "verify"`
when `merge_needs_verify == true`.
## MEDIUM fixes
7. **`do_closeout("merge", :merged)` forgot to reset `merge_needs_verify`**.
8. **`escalate_to_human/3` forgot all three new fields** (folded into #1).
9. **`File.exists?` accepted directories** at script path. Now uses
`File.Stat type: :regular` check.
10. **`verifier_test.exs` "not executable" test was hollow** — it
passed `:commands` which short-circuits the executable check,
so the test would have passed even if the executable check were
completely removed. Rewritten to actually exercise the gate.
Plus new tests: directory at script path → ignored; sanity that
normal path + executable script DOES fire ci-parity.
## Tests
3 new regression-guard tests for the bug class codex caught:
- `workspace_and_config_test`: parses ALL new schema fields from
WORKFLOW.md round-trip — would have caught the missed
`extract_*_options` updates within seconds.
- `dispatch_resolver_test`: `verify_drift_escalated=true` short-
circuits `verify_rule` (positive + negative).
Total: 401 tests, 0 failures. mix compile --warnings-as-errors clean.
Escript rebuilt.
## Out of scope (filed mentally for follow-up)
- LOW-2: `@max_merge_sync_cycles` and `@max_mergeability_unknown` are
still hardcoded module attrs. Should be Config-driven for symmetry
with the other tunables. Separate small PR.
- HIGH-3 (aggressive): replay_current_unit_rule loses subtask_text on
any retry-with-text subtask (verify-fix-1, rework-1, merge-sync-1).
Pre-existing bug, not introduced by this branch — separate PR.
claudioccm
referenced
this pull request
in claudioccm/symphony
May 2, 2026
Covers all five Tracker callbacks against a mocked client, the module-scoping URL path that's the load-bearing regression guard for Plane API quirk #1 (module filter params don't work on /work-items/), the markdown→HTML transform on comments, and Issue.from_payload/2 priority enum mapping + branch_name slug synthesis. 71 new tests across: - test/symphony_elixir/plane/adapter_test.exs (25 tests) - test/symphony_elixir/plane/client_test.exs (27 tests) - test/symphony_elixir/plane/issue_test.exs (19 tests) mix.exs ignores Plane.Client in coverage (HTTP boundary, parity with Linear.Client which is also ignored). All other Plane modules at 100%. Refs: PRO-23
3 tasks
chihsuan
referenced
this pull request
in Automattic/symphony
May 15, 2026
…ject-routing-for-multi-repo RSM-1779: Add label-based workflow routing
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.
Context
Unify env-variable interpolation so
WORKFLOW.mduses$VARonly.TL;DR
Replace
env:config indirection with$and update docs/specs accordingly.Summary
env:resolution in config path and secret resolution paths.$env references and literalenv:behavior.$as the only config indirection syntax.Alternatives
env:compatibility was intentionally rejected to complete a clean cutover with no migration path.Test Plan
mix test test/symphony_elixir/core_test.exs test/symphony_elixir/workspace_and_config_test.exsmix specs.checkmake -C elixir all