Skip to content

fix: keep structured gh output free of terminal color - #115

Merged
lukemaj merged 2 commits into
mainfrom
fix/93-gh-structured-color
Oct 18, 2026
Merged

lukemaj merged 2 commits into
mainfrom
fix/93-gh-structured-color

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

What: Structured GitHub CLI calls keep JSON free of terminal color, and dispatch now reports actionable errors for empty or malformed stdout.
Why: Forced color/TTY settings contaminated JSON; dispatch also returned null for empty output or leaked a bare JSONDecodeError for malformed output.
So what: The coordinator owns the next independent review of this exact head and any merge decision; this is an implementation handoff.

Closes #93

Objective contribution: reliable structured GitHub output preserves trustworthy task evidence.

Changes:

  • Both structured callers use a copied child environment with NO_COLOR=1, CLICOLOR=0, CLICOLOR_FORCE=0 and GH_FORCE_TTY removed. Parent environment and payload content are preserved.
  • Dispatch names the shell-quoted command and stderr (or (no stderr)) when successful gh returns empty, whitespace-only or non-JSON stdout. Malformed JSON retains JSONDecodeError as its explicit cause. Environment tokens are not included in the diagnostic.
  • Publish already provides command/stderr diagnostics for empty and malformed output; its existing real-subprocess tests pass. Its fix: explain PR task form for empty Issue publish scopes #110 empty-scope changes remain intact.
  • Added dispatch regression coverage for all three invalid-output forms, with and without stderr, token exclusion and exception chaining. Existing synthetic subprocess tests cover forced color, forced TTY and payload preservation.

Candidate:

Proof on the exact committed head:

  • Failing first: python3 -m unittest -v tests.test_gh_environment.StructuredGhEnvironmentTests.test_dispatch_invalid_stdout_names_command_and_stderr on the rebased pre-fix code produced 2 failures (empty output returned null) and 4 errors (bare JSONDecodeError). The same test passes after the fix.
  • python3 -m unittest -v tests.test_gh_environment tests.test_publish_gh tests.test_dispatch tests.test_publish: 41 passed.
  • python3 -m unittest discover -s tests: 810 passed in 71.944s, Python 3.14.7.
  • python3 -m compileall -q agent_observer tests scripts: passed.
  • bin/agent-observer --version: 1.2.6; git diff --check: passed; worktree clean.
  • Exact-head CI: success.
  • Prior forced-color/TTY failing-first subprocess evidence remains recorded in fix: keep remaining structured gh output free of terminal color #93 for the original implementation.

Luna r1 full-suite discrepancy:

  • Full suite on current main f57824ea180194c317753f99469e9722213840eb, in an isolated checkout with Git history: 806 passed in 63.568s, same host/Python. None of the four reported failure areas reproduced on main or the candidate.
  • tests.test_placement, tests.test_tool_arguments, tests.test_writer_lock: all 40 tests also passed in 4.777s with BOTH dispatch/publish structured_env entry points patched to raise if invoked. No invocation occurred.
  • Those implementation and test files are unchanged from main. structured_env copies os.environ and is used only for gh children; placement git and writer-lock subprocess paths do not use it.
  • Load sensitivity is plausible: the argument timing assertion is 1s, placement Git subprocess timeout is 5s, and lock tests use bounded 2s/5s waits. Host load averages during investigation were approximately 5-8 on 8 CPUs. The earlier failures' exact environmental cause remains unconfirmed; no evidence supports a regression from this PR, so no unrelated fixes were made.
  • An initial baseline archive run had nine historical-upgrade setup errors because it lacked Git history. It was replaced by the successful history-backed baseline above; the archive run is not claimed as full baseline proof.

Limits:

  • No independent review was run by this implementation sub-agent. Luna r1's P2 is addressed; the new exact head still needs coordinator-owned independent review.
  • No merge, VERSION, plugin manifest, CHANGELOG, workflow/settings, installation, live-ledger writes or publication POSTs. Versioning remains deferred per explicit task instructions.
  • Cost comment remains unverified: the earlier isolated-ledger attempt reported nothing to publish: no sessions in scope. No new publish attempt was made.
  • Website impact: none, this changes CLI child environments and diagnostics only.

Requirements and who asked: User, #93 and Luna r1 require plain structured output, actionable command/stderr errors, failing-first proof, parent/content preservation, current-main rebase and investigation of four reported suite failures.
Deleted: ANSI stripping, shared parser refactoring, unrelated flaky-test fixes without causal evidence, live publication proof and release-file changes.
Bottleneck: Invalid gh stdout lacked actionable dispatch diagnostics; exact-head and baseline proof now distinguish that defect from the unconfirmed suite discrepancy.
Checked myself: Read #93, #115 and Luna r1; inspected both structured callers and full diff; observed failing-first diagnostics, 41 focused passes, 810 candidate passes, 806 main passes, 40 helper-exclusion passes, compileall, CI and remote identity.

Implemented with Codex (gpt-6.1-sol, medium reasoning) through T3 Code.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Luna review r1 of 476adee: REQUEST CHANGES

  • P2 — agent_observer/dispatch.py:259: If gh exits successfully but emits malformed or empty stdout (for example, a future CLI/API response or wrapper writes a diagnostic to stdout), _gh_json lets json.loads raise a bare JSONDecodeError. Unlike publish._gh, it omits the command and stderr, so the failure is not actionable. Wrap the parse error with the command and stderr while preserving the cause/context.

The environment helper itself is sound: it copies the parent environment (retaining PATH, GH_TOKEN, GH_HOST, and proxy variables), overrides terminal color flags and removes GH_FORCE_TTY only for the child. Both structured callers use it; the PR leaves VERSION, manifests and CHANGELOG untouched. The subprocess tests cover both forcing cases and verify parent environment/content preservation.

@lukemaj
lukemaj force-pushed the fix/93-gh-structured-color branch from 476adee to f82c419 Compare October 18, 2026 23:07
@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Luna review r2 of f82c419: APPROVE

No actionable findings.

  • The P2 in dispatch._gh_json is fixed: empty/whitespace stdout raises an actionable RuntimeError; malformed stdout includes the command and stderr and preserves JSONDecodeError as the cause.
  • Regression proof: tests.test_gh_environment.StructuredGhEnvironmentTests.test_dispatch_invalid_stdout_names_command_and_stderr passes on this head and produced 2 failures and 4 errors when run against the pre-fix behavior. The error assertions include a synthetic GH_TOKEN and confirm it is absent from the message.
  • The previously failing areas pass individually: placement (19), tool arguments (14), writer lock (7). The complete suite passes (810 tests).
  • The publish.py refactor retains the behavior merged in fix: explain PR task form for empty Issue publish scopes #110. No VERSION, manifest, or CHANGELOG changes.

@lukemaj
lukemaj merged commit 49486ed into main Oct 18, 2026
2 checks passed
@lukemaj
lukemaj deleted the fix/93-gh-structured-color branch October 18, 2026 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: keep remaining structured gh output free of terminal color

1 participant