fix(cli): derive repo log levels#407
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
moncher-dev
changed the title
![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: 85f848f3ce
ℹ️ 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".
| case "run-failed": | ||
| case "worker-error": | ||
| case "hook-failed": | ||
| return "error"; |
There was a problem hiding this comment.
When a worker turn fails, the orchestrator writes a structured run event with event: "turn_failed" (checked packages/orchestrator/src/service.ts:2347-2351), but this derived-level switch falls through to info for that event. In runs where the failure recorded in events.ndjson is a turn failure, gh-symphony repo logs --level error hides the failure and may report no error matches, which defeats the new level filter for a real runtime failure. Add turn_failed to the error cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in aaa1855: turn_failed is now classified as error, and the CLI logs regression test asserts --level error includes turn_failed output.
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Project #14 watchdog review: no blocking findings found.
Verification performed:
- Claude Code read-only review recommended approve and found no blockers.
- Smoke test passed:
pnpm --filter @gh-symphony/cli test(35 test files, 440 tests). - Evidence bundle: https://evidence-browser.dev0.daniworks.kr/w/github-symphony/b/project14-pr-407-20260622-143334
Note: I am approving the PR review, but I am not moving the project item to Land because this changes CLI runtime behavior under packages/cli/**, so it does not pass the low-risk Land gate for automatic landing.
There was a problem hiding this comment.
Pull request overview
This PR fixes gh-symphony repo logs --level filtering by deriving a log level from the structured event discriminator (since events don’t have an event.level field) and improves CLI UX by emitting a notice when --level yields no matches. This is primarily a CLI/observability consumption change (it does not alter the event schema or event writers).
Changes:
- Derive
error | warn | infofromevent.eventwhen applying--levelfiltering inrepo logs. - Track whether any events matched the
--levelfilter and print a no-match notice instead of returning empty output. - Add tests covering derived-level filtering and the no-match notice behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/cli/src/commands/logs.ts | Derives a LogLevel from event discriminators, applies it to --level filtering, and reports no-match cases. |
| packages/cli/src/commands/logs.test.ts | Adds regression tests for derived --level filtering and no-match notice output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getLevel(event: LoggedEvent): LogLevel { | ||
| switch (event.event) { | ||
| case "run-failed": | ||
| case "worker-error": | ||
| case "hook-failed": | ||
| return "error"; | ||
| case "run-suppressed": | ||
| case "run-retried": | ||
| return "warn"; | ||
| default: | ||
| return "info"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolved in aaa1855: added turn_failed to the derived error-level cases and covered it in logs.test.ts.
| try { | ||
| const content = await readFile(eventsPath, "utf8"); | ||
| const lines = content.trim().split("\n").filter(Boolean); | ||
| let matchedEvents = 0; | ||
| for (const line of lines) { |
There was a problem hiding this comment.
Resolved in aaa1855: --level is validated once before reading logs. Unsupported values now fail fast with Unknown --level "<value>". Valid values: error, warn, info. and exit code 1.
| let foundRuns = false; | ||
| let matchedEvents = 0; | ||
| const events: LoggedEvent[] = []; |
There was a problem hiding this comment.
Resolved in aaa1855: validation now happens once before both the --run path and scan path, so unsupported levels never reach either filtering path.
| }); | ||
|
|
There was a problem hiding this comment.
Resolved in aaa1855: added a regression test for --run <id> --level error where the run has events but none match the derived level.
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
✅ Approve — completeness verification (watchdog review)
Requirements coverage (issue #386): Fully met.
getLevelnow deriveserror | warn | infofrom the event discriminator using exactly the mapping proposed in the issue (error← run-failed/worker-error/hook-failed,warn← run-suppressed/run-retried,infootherwise), and the existing--levelfilter consumes it (logs.ts:87,logs.ts:175). The bug — filtering on the nonexistentevent.level— is gone.- The "matches nothing" gap is closed: a
No events matched --level <x>.notice is written to stderr instead of silent empty output, in both the--runpath and the scan path. - Adds the
--leveltest coverage the issue flagged as missing.
Smoke test (black-box, built CLI against a synthetic .runtime/orchestrator):
repo logs --level error→ onlyrun-failed✓repo logs --level warn→ onlyrun-retried✓repo logs --level info→run-started+run-completed✓- no filter → all events ✓
- run with only info events +
--level error→No events matched --level error.on stderr (no silent empty) ✓ --run <id> --level warnpath → filters correctly ✓- unknown level (
--level debug) → graceful notice, no crash ✓
Quality: pnpm --filter @gh-symphony/cli test (8/8 in logs.test.ts), typecheck, and lint all clean locally; CI Test + Container Smoke green. Scope is appropriately minimal (CLI read/filter only — no event-schema change), avoiding over-engineering. Two non-blocking nits left inline (level-value validation, notice attribution).
No blockers. Approving.
Generated by Claude Code
|
|
||
| function getLevel(event: LoggedEvent): string | undefined { | ||
| return typeof event.level === "string" ? event.level : undefined; | ||
| function getLevel(event: LoggedEvent): LogLevel { |
There was a problem hiding this comment.
Nit (non-blocking): getLevel now returns a closed error | warn | info set, but parsed.level is still an arbitrary string compared with getLevel(event) !== parsed.level. A misspelled or unsupported value (--level Error, --level debug) silently falls through to the "No events matched" notice. Since the whole point of #386 was that operators get misled by empty output, consider validating --level against the three accepted values up front and surfacing them (e.g. Unknown --level "debug". Valid values: error, warn, info.). Not required for this fix.
Generated by Claude Code
There was a problem hiding this comment.
Accepted and resolved in aaa1855: --level now validates against error, warn, and info up front, with a clear error and nonzero exit for unsupported values.
| process.stderr.write("No runs found. Start the orchestrator first.\n"); | ||
| return; | ||
| } | ||
| if (parsed.level && matchedEvents === 0) { |
There was a problem hiding this comment.
Nit (non-blocking): matchedEvents is incremented after all three filters (project, level, issue), but the notice attributes an empty result solely to --level. So repo logs --level error --issue does/not#exist prints "No events matched --level error." even when it was the --issue filter that excluded everything. Minor wording accuracy only — fine to leave for the common case.
Generated by Claude Code
There was a problem hiding this comment.
Accepted and resolved in aaa1855: the no-match notice now says No events matched the provided filters (--level <level>)., so it no longer attributes an empty result solely to the level filter when other filters are also present.
|
Rework complete for the review feedback on #407. Resolved in
Local validation passed: |
moncher-dev
force-pushed
the
feat/386-repo-logs-level
branch
from
aaa1855 to
25f49a5
Compare
TL;DR
gh-symphony repo logs --levelnow filters historical structured events by a derived log level instead of reading the nonexistentevent.levelfield. It also treats worker turn failures as errors, rejects unsupported level values, and reports empty filtered results without implying only--levelwas responsible.변경 지점 다이어그램
repo logsargs -> validate--level-> run event NDJSON -> derive level fromeventdiscriminator -> apply filters -> format matching events or print no-match notice여기부터 보세요
packages/cli/src/commands/logs.ts: validates--level, deriveserror | warn | infofrom event type, includesturn_failedin error-level output, and reports no matches for filtered log views.packages/cli/src/commands/logs.test.ts: covers scanned logs, explicit--run, warning/error filters,turn_failed, unsupported levels, and no-match notice behavior..changeset/repo-logs-level-filter.md: patch changeset for the user-visible CLI behavior change.위험 & 롤백
Risk is low: the change is limited to CLI read/filter behavior for existing event logs and does not alter event schema or append sites. Roll back by reverting the CLI helper, tests, and changeset in this PR.
변경 파일
.changeset/repo-logs-level-filter.mdpackages/cli/src/commands/logs.tspackages/cli/src/commands/logs.test.tsEvidence
pnpm --filter @gh-symphony/cli test -- src/commands/logs.test.ts— passed, 10 testspnpm lint— passedpnpm test— passedpnpm typecheck— passedpnpm build— passedIssues — Closed #386
Closes #386
머지 후/사람 확인
gh-symphony repo logs --level errorshows failed historical events in a real runtime directory.repo logsfilters regress for operators.