fix(cli): report effective workflow timeouts - #884
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: fc43c5bd3d
ℹ️ 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".
moncher-dev
left a comment
moncher-dev
left a comment
There was a problem hiding this comment.
@hojinzs ready for your review — verified at head fc43c5bd. This one I exercised for real rather than reading; all three checks pass.
Build
pnpm install --frozen-lockfile && pnpm build && pnpm test in a clean worktree of this head. One failure, core > executeWorkspaceHook > drains large stdout without waiting for the timeout (expected 'timeout' to be 'success') — a load-sensitive timing test on a host at load 21, in a package this PR does not touch (the diff is packages/cli only). Re-run in isolation: passes in 543ms. CI is green on both jobs. Flake, not this change.
packages/cli/src/commands/workflow.test.ts: 22 passed.
1. The reported bug is fixed
Ran the built CLI from both branches against the real ~/moncher-stack/github-symphony/WORKFLOW.md, which declares runtime.timeouts.read_timeout_ms: 30000 / stall_timeout_ms: 900000:
main codex.read_timeout_ms=5000 <- wrong
codex.stall_timeout_ms=300000 <- wrong
PR884 runtime.timeouts.read_timeout_ms=30000 <- matches WORKFLOW.md
runtime.timeouts.stall_timeout_ms=900000
The runtime was always using 30000/900000; only the report was wrong. It now agrees.
2. Legacy shape still reports its own values
Built a fixture with a top-level codex: block only (7777 / 88888 / 999999) and no runtime: block:
main codex.read_timeout_ms=7777
PR884 runtime.timeouts.read_timeout_ms=7777
Preserved.
3. The label no longer lies about its source
codex.read_timeout_ms= is gone for values that came from runtime.timeouts. The label now names the effective slot.
Two things for you to decide
a) No changeset. CLAUDE.md: "Behavior-changing PRs must add a changeset (.changeset/*.md, package @gh-symphony/cli)." This changes released workflow validate output and adds none — #886 added one for a comparable change. As it stands this fix either rides an unrelated release unattributed, or fails to trigger one. The Codex reviewer flagged the same. Your call given #883 is open.
b) --json schema change. summary.codex.{readTimeoutMs,stallTimeoutMs,turnTimeoutMs} are removed and replaced by summary.runtimeTimeouts.*. Confirmed against --json output. Codex asked for compatibility aliases; I'd argue against them — the old keys held a value the runtime did not use, so a consumer reading a now-missing key fails loudly instead of silently trusting a wrong number. But it is a breaking change to a released command's output and worth being deliberate about.
Separate latent bug found while testing (not this PR's)
resolveWorkflowRuntimeTimeouts returns runtime?.timeouts ?? codex, but the parser materializes runtime.timeouts with defaults whenever a runtime: block is present. So the ?? codex fallback is dead for any workflow that has a runtime: block without a timeouts: sub-block:
front-matter: runtime: {kind: codex-app-server, ...} + codex: {read_timeout_ms: 7777, ...}
parsed runtime.timeouts -> {readTimeoutMs: 5000, stallTimeoutMs: 300000} # defaults
resolved -> 5000 / 300000 # 7777 silently discarded
The orchestrator forwards this resolver, so the runtime also ignores the declared 7777 — the report is right and the behavior is wrong. Only reachable mid-migration (both blocks present), but it fails silently. Filing it separately.
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Reviewed at head fc43c5bd. Built the CLI and exercised workflow validate against four front-matter shapes rather than reading the diff alone. All six acceptance criteria of #882 are met, and the reported defect is genuinely fixed — but the new runtimeTimeouts field leaks the whole codex block into --json, so I'm holding on that one line.
Smoke test — packages/cli/dist/index.js workflow validate
| shape | reported | verdict |
|---|---|---|
this repo's WORKFLOW.md (runtime.timeouts declared 30000/900000/3600000) |
30000 / 900000 / 3600000 |
matches the declared values — AC 1, 2 met, the #882 defect is gone |
legacy codex: only (7777/88888/999999) |
7777 / 88888 / 999999 |
no regression — AC 3 met |
| neither block | 5000 / 300000 / 3600000 |
documented defaults — AC 4 met |
runtime: without timeouts: + codex: values |
5000 / 300000 / 3600000 |
truthful; see the test-file comment |
AC 5 (a test pins precedence) and AC 6 (the label no longer says codex.*) are both satisfied — the it.each block would fail if the report were rewired back to workflow.codex, and not.toContain("codex.read_timeout_ms=") pins the label.
Scope is clean: five files, no speculative refactor, the shared resolver reused rather than re-implemented, and the tempting-but-out-of-scope precedence change correctly left alone. packages/cli/src/commands/workflow.test.ts 22/22 pass; pnpm lint, pnpm typecheck, pnpm build pass; CI green on both jobs. (pnpm format flags 72 files here, all untouched by this PR — a trailing-comma drift in my container, not yours. This PR's five files pass prettier --check.)
Findings
- P1 —
runtimeTimeouts: resolveWorkflowRuntimeTimeouts(workflow)(workflow.ts:885). On the fallback branch the resolver returns the wholeworkflow.codexobject; the declared three-field type doesn't survive to runtime, and--jsonserializes it verbatim.summary.runtimeTimeoutshas 3 keys with aruntime:block and 7 without — includingcommandandapprovalPolicyduplicated under a field named "timeouts". Inline comment has the evidence and a one-line fix. This is the only thing blocking. - P3 — the label misattributes in the reverse direction for legacy-only files (
runtime.timeouts.read_timeout_ms=7777for a value declared undercodex:). Suggested a(source: codex)suffix. Non-blocking. - P3 — the mixed shape (row 4 above) has no test. Suggested a fourth
it.eachentry.
On the two open questions
a) Changeset — I'd add one. Your reading is defensible: the land gate is "changeset present if labeled" (.codex/skills/land/SKILL.md:131) and #882 carries no label, so nothing will block you. But that's the enforcement threshold, not the authoring rule — CLAUDE.md says "Behavior-changing PRs must add a changeset" and CONTRIBUTING.md says "when the change affects released package behavior." This changes released workflow validate output in two ways. With fuzzy-cats-guard.md pending, this fix ships in that release with no changelog line, and the operator who hit #882 has no way to tell which version fixed it. A patch changeset costs one file.
b) --json compatibility aliases — I agree with you, no aliases. The old keys held a value the runtime did not use; a consumer reading a now-missing key fails loudly instead of silently trusting a wrong number, which is the whole point of #882. Codex's concern is real but the alias is the worse resolution. What's missing is that the break is undocumented: all three doc edits describe the text output only, and none of README.md, packages/cli/README.md, or docs/configuration.md mentions that summary.codex.{read,stall,turn}TimeoutMs moved to summary.runtimeTimeouts.*. Put it in the changeset from (a) and a line in packages/cli/README.md. Fixing the P1 first makes that documented schema stable rather than shape-dependent.
The latent core bug you found
Confirmed independently — parser.ts:1143-1151 materializes runtime.timeouts with defaults for any runtime: block, so ?? workflow.codex in resolveWorkflowRuntimeTimeouts is unreachable whenever a runtime: block exists, and a declared codex.read_timeout_ms is silently discarded by the runtime, not just the report. Right call keeping it out of this PR — #882 puts the precedence explicitly out of scope. Please link the follow-up issue here when you file it.
Fix the P1 and take or wave off the two P3s and I'll approve on the next pass.
Generated by Claude Code
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Approve. Round 2. Every P1/P2 from round 1 is fixed, and I verified each one against the built CLI at 2a51e85 rather than taking the resolution comments at their word.
Acceptance criteria (#882)
| # | Criterion | Verified |
|---|---|---|
| 1 | Reports resolveWorkflowRuntimeTimeouts, not workflow.codex |
✅ |
| 2 | runtime.timeouts declared → reported = declared |
✅ this repo's WORKFLOW.md → 30000 / 900000 / 3600000 |
| 3 | Legacy codex: only → no regression |
✅ 7777 / 88888 / 999999 |
| 4 | Neither → documented defaults | ✅ 5000 / 300000 / 3600000 |
| 5 | Test pins precedence, fails on a rewire to workflow.codex |
✅ 4-shape it.each, values + source + exact JSON key set |
| 6 | Label doesn't misattribute the source |
The defect in the issue is gone: the exact command and file that reproduced it now print the declared values.
Smoke test
Built at this head and ran workflow validate across all four front-matter shapes, text and --json:
legacy codex only → 7777/88888/999999 (source: codex/defaults)
runtime.timeouts → 30000/900000/3600000 (source: runtime.timeouts)
runtime, no timeouts, + codex → 5000/300000/3600000 (source: runtime.timeouts)
neither → 5000/300000/3600000 (source: codex/defaults)
The round-1 P1 leak is confirmed fixed: summary.runtimeTimeouts is exactly {readTimeoutMs, stallTimeoutMs, turnTimeoutMs} in all four shapes — three keys, never seven — and summary.codex no longer carries timeout keys at all. Grepped the tree for stale references to the removed keys; docs and tests are consistent.
pnpm lint, pnpm typecheck, pnpm build, and packages/cli/src/commands/workflow.test.ts (23 tests) all pass locally. CI green on 2a51e85 (Test, Container Smoke).
Scope
Tight and appropriate. One resolver call, a three-field narrowing, one provenance field, docs, changeset — no over-engineering. The precedence itself is untouched and correctly deferred to #888, and the fourth test case pins today's behavior so that fix has to be a deliberate decision rather than a silent follow.
Non-blocking
Three P3s inline: per-field provenance (codex/defaults can't describe a partially-declared block), the source label being re-derived rather than returned by the resolver, and patch vs minor for the --json key removal. None is worth another round — take them as follow-ups or reply and drop them.
Generated by Claude Code
| runtime.timeouts.read_timeout_ms=${report.summary.runtimeTimeouts.readTimeoutMs} (source: ${report.summary.runtimeTimeoutSource}) | ||
| runtime.timeouts.stall_timeout_ms=${report.summary.runtimeTimeouts.stallTimeoutMs} (source: ${report.summary.runtimeTimeoutSource}) | ||
| runtime.timeouts.turn_timeout_ms=${report.summary.runtimeTimeouts.turnTimeoutMs} (source: ${report.summary.runtimeTimeoutSource}) |
There was a problem hiding this comment.
P3 — codex/defaults conflates two different answers, and per-report granularity can't express a partially-declared block.
The source label is one value for all three lines, so it can only say "not runtime.timeouts". Verified against the built CLI at this head with a codex: block declaring only read_timeout_ms:
codex:
read_timeout_ms: 7777runtime.timeouts.read_timeout_ms=7777 (source: codex/defaults)
runtime.timeouts.stall_timeout_ms=300000 (source: codex/defaults)
runtime.timeouts.turn_timeout_ms=3600000 (source: codex/defaults)
7777 came from codex:; 300000 and 3600000 are defaults. Three lines, one label, two genuinely different provenances. The operator who greps WORKFLOW.md for stall_timeout_ms after reading "codex" finds nothing — the smaller version of the loop #882 describes.
This is a real improvement over the previous head and I'm not blocking on it, but the criterion asked for the source to be unambiguous, and per-field provenance is what actually delivers that: resolve each field and label it codex or default individually. Worth a follow-up rather than another round here.
nit, same lines: (source: …) repeated identically three times is noise while the label stays per-report — a single runtime.timeouts source=… line above the three values reads better.
Generated by Claude Code
| runtimeTimeoutSource: workflow.runtime | ||
| ? "runtime.timeouts" | ||
| : "codex/defaults", |
There was a problem hiding this comment.
P3 — the source is re-derived here instead of coming from the resolver, so it can silently drift when #888 lands.
resolveWorkflowRuntimeTimeouts branches on workflow.runtime?.timeouts; this branches on workflow.runtime. They agree today, and not by luck — WorkflowRuntimeConfig.timeouts is a required field (config.ts:131) and the parser always materializes it (parser.ts:1143-1151), so runtime truthy ⟺ runtime.timeouts present. (Which also means the ?? workflow.codex arm of the resolver is reachable only when runtime is absent entirely — the "runtime block, no timeouts sub-block" shape never falls back, exactly as your fourth test case pins.)
The drift risk is that this is a duplicated invariant rather than a derived one. #888 is about changing precisely that precedence; when it changes the resolver, nothing here fails to compile and no test in this file necessarily fails — the label just starts lying again, in the same class of way #882 was filed for.
Cheap insurance, and it makes the follow-up in the sibling comment fall out for free: have the resolver return the provenance alongside the values (or add a thin resolveWorkflowRuntimeTimeoutSource next to it in core), so the label and the number are read from one place.
Non-blocking — the current output is correct and verified.
Generated by Claude Code
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@gh-symphony/cli": patch | |||
There was a problem hiding this comment.
P3 — patch understates a removal of released --json keys.
I agree with your call on the codex bot's alias request: summary.codex.*TimeoutMs could report values the runtime never used, so aliasing them forward would preserve exactly the failure #882 exists to kill. Removing them is right.
The severity is the separate question that thread didn't cover. summary.codex.{readTimeoutMs,stallTimeoutMs,turnTimeoutMs} were in released workflow validate --json output and are now absent — I confirmed against the built CLI at this head that summary.codex carries only approvalPolicy, threadSandbox, turnSandboxPolicy. Automation reading those keys gets undefined, not a wrong number, which is the better failure but still a break.
A minor bump would put the removal in the changelog where a consumer pinning ~ actually sees it before it bites. The one-line body is otherwise good and does mention the new object.
Your call — if CLI --json output isn't covered by the package's compat surface, say so and this drops.
Generated by Claude Code
2 participants
Issues
Summary
workflow validatereport the effective timeout values used by the runtime.codexfallback, parser-materialized runtime defaults, and documented defaults.Change-point diagram
WORKFLOW.md→ core parser →resolveWorkflowRuntimeTimeouts→ narrowed CLI validation summary → text/JSON reportStart here
packages/cli/src/commands/workflow.ts:847— resolves and narrows effective runtime timeouts for the report.packages/cli/src/commands/workflow.test.ts:125— pins precedence, all four front-matter shapes, sources, and the JSON key set.User-Visible Behavior / Operational Impact
runtime.timeouts.*and identifiesruntime.timeoutsversuscodex/defaults.summary.codex.*TimeoutMsfields tosummary.runtimeTimeouts.*; no compatibility aliases preserve the incorrect fields.Validation
pnpm exec prettier --check packages/cli/src/commands/workflow.ts packages/cli/src/commands/workflow.test.ts packages/cli/README.md .changeset/tidy-pandas-report.md— passpnpm exec vitest run packages/cli/src/commands/workflow.test.ts— pass (23 tests)WORKFLOW.mdsmoke validation — pass (30000,900000,3600000, sourceruntime.timeouts)pnpm lint— passpnpm test— passpnpm typecheck— passpnpm build— passChangeset
.changeset/tidy-pandas-report.md(patch)Risks & rollback
2a51e85andfc43c5b.Changed files
packages/cli/src/commands/workflow.ts— report narrowed effective timeouts and their source.packages/cli/src/commands/workflow.test.ts— cover precedence, fallback, defaults, mixed runtime configuration, labels, and JSON shape.README.md— document effective validation output.packages/cli/README.md— document timeout semantics and the JSON key migration.docs/configuration.md— document timeout precedence and worker parity..changeset/tidy-pandas-report.md— release-note the CLI behavior fix.Post-merge / human validation
Security
.envfiles, or generated installation tokens are committed