Skip to content

feat(#7303): show cron responses before run diagnostics - #7329

Open
rodboev wants to merge 8 commits into
nesquena:masterfrom
rodboev:pr/7303-cron-response-first
Open

rodboev wants to merge 8 commits into
nesquena:masterfrom
rodboev:pr/7303-cron-response-first

Conversation

@rodboev

@rodboev rodboev commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

  • Tasks already extracts the response for a collapsed run, but expansion switches to the complete saved artifact and puts prompt and loaded context ahead of the result.
  • Response detection also exists in multiple places, so script output and marker-like text can be interpreted differently by the server and browser.
  • The accepted direction in UX: show cron responses before raw prompt and execution context #7303 (comment) keeps one strict projection, makes response primary, and preserves diagnostics and exact raw output through progressive disclosure.

What Changed

  • api/cron_output.py: adds the shared fail-closed response, diagnostics, and raw artifact projection, and resolves each saved run's Agent or script mode from its immutable artifact envelope. Current-job fallback is limited to unclassified legacy files.
  • api/routes.py: applies per-file mode resolution to history, detail, and bounded-output routes while preserving exact raw content, usage data, limits, path checks, and legacy Agent marker-only behavior.
  • static/panels.js: uses one projection for collapsed and expanded response views and adds separate diagnostics and raw disclosures.
  • static/style.css: keeps the response hierarchy readable across desktop and narrow Tasks layouts.
  • static/i18n.js: localizes the new disclosure and raw-output controls across all supported locales.
  • tests/test_issue7303_cron_response_first.py: covers parser fallbacks, historical mode edits, deleted jobs, legacy fallback, marker-like script payloads, route fidelity, response expansion, accessibility, focus, and responsive rendering.
  • tests/test_issue2289_cron_detail_expansion.py, tests/test_sprint10.py, and tests/test_v050257_opus_followups.py: preserve expansion, bounded-output, locale, and path-security contracts.

Why It Matters

Scheduled reports become readable as reports instead of raw execution logs. Prompt and execution context stay available for debugging, and the exact saved artifact remains accessible without displacing the result.

Verification

The historical-mode regression fails on the pre-rework PR head because saved files inherit the current job's mode, and passes after per-file artifact resolution. Focused parser and route checks cover Agent-to-script and script-to-Agent edits, deleted and legacy jobs, failed and silent scripts, fenced and marker-like payloads, exact raw fidelity, usage extraction, and bounded windows; the existing headless browser, accessibility, locale, security, and narrow-layout checks remain green.

Risks / Follow-ups

Artifacts matching the current Agent and script envelopes are classified from their own saved metadata. Older unclassified artifacts retain the current-job fallback when that job exists; deleted or malformed producer-looking artifacts remain raw. The change preserves the existing raw artifact and file-read policy; bounded-read changes remain with #6141 . The accepted 1024x600 before/after capture is retained because this rework changes backend classification only; broader viewport sign-off remains external.

Contract Routing

Task type: Tasks UX and product-semantics enhancement.

Touched areas: cron artifact mode ownership, output parsing, run-detail API projection, responsive and accessible Tasks presentation.

Relevant public docs:

  • docs/UIUX-GUIDE.md
  • docs/CONTRACTS.md

Scope boundaries: Historical presentation reads immutable fields already present in saved Agent and script artifacts; it does not change Agent output persistence, retention, filenames, schemas, or file-read bounds.

Evidence needed before claiming done: pre-rework red and post-rework green mode-mutation proof, focused fallback and security checks, the existing headless layout sweep, and the published before/after screenshots.

Upstream

Closes #7303.

Screenshots

Before, expanding a run places prompt and context ahead of the response.

Before

After, the response stays primary while diagnostics and raw output remain available.

After

Model Used

GPT-5 via Codex CLI with headless Playwright layout and screenshot verification

@nesquena-hermes nesquena-hermes added size:L Large PR (>10 files or >250 LOC) ux User experience / visual polish labels Aug 27, 2026
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

Reading the full parser, all route and browser changes, the changed regression files, and the Agent cron artifact contract, I found one blocking historical-run bug. The parser itself is conservative about fences, quotes, duplicate markers, errors, and malformed artifacts. The problem is how the route chooses job_mode: it uses the current job record rather than metadata for the historical run being opened. Since cron jobs can be edited between runs, an old script run can later be treated as an agent run, or an old agent run can be forced to raw mode.

Code reference

api/routes.py:21733-21766 resolves the current job and passes its present mode into the projection:

get_job = getattr(cron_jobs, "get_job", None)
job = get_job(job_id) if get_job else None
job_mode = "unknown" if job is None else (
    "script" if job.get("no_agent") else "agent"
)
projection = parse_cron_output_artifact(content, job_mode=job_mode)

The same current-state lookup is repeated for history and output windows at api/routes.py:21714-21725 and api/routes.py:21877-21891. Meanwhile, static/panels.js:1596-1676 carries no_agent through the edit form, so mode is not immutable for the lifetime of files already under run history.

api/cron_output.py:32-68 correctly refuses script and unknown modes, but that safety only works when the supplied mode belongs to this artifact. tests/test_issue7303_cron_response_first.py:120-141 fixes get_job() to no_agent=False; it does not cover a historical mode change.

Diagnosis / recommendation

Do not derive historical presentation from the mutable current job definition. Persist the run mode beside each output filename, or derive it from an immutable field in the saved artifact contract and use the current job only as a fallback for legacy files. Deleted jobs should remain unknown/raw unless their own run metadata proves agent mode.

A minimal regression should create or fixture two historical artifacts under one job, change the current job from script to agent and then from agent to script, and verify that each artifact keeps its original projection. Also cover a deleted job and a script payload containing marker-like headings; both must remain raw.

The browser response-first hierarchy at static/panels.js:1425-1506 looks sound once the API projection is trustworthy. Diagnostics and raw output use literal DOM text, expansion changes only the response cap, and the tests cover focus plus narrow layout.

Verification

Exact-head CI is green, but the current tests do not exercise mode mutation. No contributor-authored code was executed during this read-only review. Verdict: block until historical run mode no longer depends on the current job record.

@rodboev

rodboev commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

The historical mode now belongs to each saved artifact. api/cron_output.py recognizes the producer-owned Agent and script envelopes, stays fail-closed for unsupported or malformed headers, and uses one forward marker scan. Current-job state is consulted only for true legacy marker-only files.

api/routes.py applies that resolution independently in history, detail, and bounded-output routes. Agent usage comes only from contiguous top-level metadata, script and unknown artifacts stay raw with no fabricated usage, and detail paths remain confined to the requested job directory.

The regressions cover both job-edit directions, deleted jobs, legacy Agent fallback, failed and silent scripts, fenced and marker-like payloads, platform-stable raw bytes, usage isolation, cross-job path traversal, and all three route consumers.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

I re-pulled the current head into a read-only worktree and reviewed the complete parser, all route consumers, the browser projection, the changed regressions, and the Agent cron artifact envelope. The historical-mode blocker from my previous review is resolved. History, detail, and bounded-output routes now classify each saved artifact before consulting mutable job state; the current job mode is used only for genuinely legacy files. The parser also fails closed for producer-looking but malformed artifacts.

Code reference

The new ownership boundary is in api/cron_output.py:57-91:

if all(name in positions for name in ("job_id", "run_time", "mode")):
    if (positions["job_id"] < positions["run_time"] < positions["mode"] and
            positions["mode"] == positions["run_time"] + 1):
        return "script"
if all(name in positions for name in ("job_id", "run_time", "schedule")):
    ...
    return "agent"
return "unknown"

api/routes.py:21833-21855 then calls resolve_cron_artifact_mode(content) first and reaches _cron_legacy_job_mode() only when is_legacy_cron_artifact(content) is true. This removes the old dependency on the current editable no_agent value. The same helper is used by history at api/routes.py:21716-21731, run detail at api/routes.py:21760-21778, and bounded outputs at api/routes.py:21898-21910.

The browser now consumes that single projection at static/panels.js:1425-1507: response text remains primary, diagnostics and exact raw output are separate disclosures, and expanding changes only the response cap.

Diagnosis / recommendation

I found no new blocker. The parser preserves exact raw content, ignores fenced marker-like text, rejects ambiguous response boundaries, and does not let response prose overwrite usage metadata. The tightened run-detail containment check is also correct: api/routes.py:21750-21758 now proves the requested file remains inside the selected job directory, not merely somewhere under the global cron output root.

The conservative handling of producer-looking near misses is appropriate. A malformed current envelope remains raw rather than borrowing a possibly unrelated current job mode; deleted jobs can still classify canonical saved artifacts from their own metadata.

Verification

tests/test_issue7303_cron_response_first.py:155-259 covers both job-mode edit directions, deleted jobs, legacy fallback, all three route consumers, exact raw fidelity, and cross-job traversal. The browser test at :306-377 pins response-first rendering, lazy diagnostics/raw disclosure, focus return, and raw fallback. Exact-head checks shown in the PR are green.

No contributor-authored code was executed during this read-only review. Verdict: the previous blocker is fixed; no further blocker found.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Gate certification: RED

Certified contributor head: 35906b938b07bfc85d8038502f37f74f40799fa2

Current synthetic rebase: cc31f2caccf5bfe4f15ab21e9326683a6e382343 on current origin/master 47e394895dfe781293dc1d9ef4e10b889dca899d

The complete suite/advisor/browser gate ran at synthetic head a2946497e0e747b961940af96dcde6c08982c2ac on ae311e4377e9a9615c376d9d23853b3d74d0fa9a. Master then advanced through #7160 in unrelated Agent-runtime code. A fresh rebase produced the same aggregate contribution patch ID (a66d7af00ff7baebc6e2d134b41f5958b1283e37), and the master interdiff has no cron-handler overlap. Focused tests plus both decisive blocker probes were rerun at cc31f2ca….

Verdict

The response-first information hierarchy is a clear improvement and the prior historical-mode blocker is fixed. This exact head still introduces one file-disclosure boundary regression, one blank-result parser regression, and mobile disclosure-control UX failures. Do not merge until these are fixed and freshly re-gated.

Must fix 1: symlinked job directories escape the cron output root

api/routes.py:21738-21745 resolves <CRON_OUT>/<job_id> first and then treats the resolved target as the trusted root:

job_dir = (CRON_OUT / job_id).resolve()
fpath = (job_dir / filename).resolve()
if not fpath.is_relative_to(job_dir): ...

If the job directory itself is a symlink to an outside directory, both values resolve under the outside target and the check passes. A sandboxed production-handler probe created cron-output/job_abc -> outside, requested filename=secret.md, and received:

HTTP 200
content = OUTSIDE_SECRET_BYTES
resolved_job = .../outside

Current master rejects the same path at the cron-root boundary.

Required fix: resolve cron_root = CRON_OUT.resolve(), require the resolved job_dir to remain under that root, then retain the per-job fpath.is_relative_to(job_dir) check. Add a symlinked-job-directory regression and keep cross-job ../ rejection.

Must fix 2: whitespace-only responses render as a blank primary result

Both response branches in api/cron_output.py check if not response, so a body containing spaces/tabs/newlines is classified as a valid Agent response. Independent sandbox reproduction:

kind=agent
response='  \t\n'
fallback_reason=None
visibly_blank=True

The route sends that projection to static/panels.js, which renders a blank primary section. Current master’s preview trims it and shows (empty).

Required fix: use if not response.strip() in canonical and legacy branches, preserving the exact raw artifact and fallback_reason='empty_response'. Add parser, route, and browser regressions for spaces/tabs/CRLF-only bodies.

Must fix 3: mobile progressive-disclosure controls are undersized and inconsistent

The new responsive rule is scoped to @container rightpanel, but the Tasks main view is not inside .rightpanel, so it never applies. Real touch-enabled 390×844 measurements:

  • View diagnostics summary: 18px high;
  • View raw output summary: 28px high;
  • the two parallel disclosures use different visual treatments.

Fable confirms this from source and screenshots. It also places the full-response toggle after the usage footer rather than immediately after the primary response.

Required fix:

  1. use the app’s real Tasks breakpoint (@media(max-width:700px)) and give both summary controls plus the response toggle at least 44px hit height with flex centering;
  2. give diagnostics/raw summaries one shared disclosure style based on the existing paused-summary idiom; keep button styling only for the response-expansion action;
  3. place the full-response action immediately after .cron-run-primary, before usage metadata;
  4. use text-align:start, remove one stacked divider, and capture default-collapsed plus diagnostics-only-open mobile states.

What is correct

  • Saved-artifact mode is classified from immutable Agent/script envelopes; current job mode is legacy-only fallback. Both job-edit directions, deleted jobs, malformed producers, marker-like script output, and usage isolation are covered.
  • The response remains primary when expanded. Diagnostics and exact raw output remain separate lazy disclosures.
  • Cross-job filename traversal is rejected, raw content remains byte-exact, and history/detail/bounded routes share the same projection.
  • Same-base real-browser proof: master’s View full output replaces the response with a 2,316-character prompt-first raw artifact; this PR keeps a 600/971-character response primary and lazily exposes diagnostics/raw. Desktop and mobile runs have zero console/page errors.

Gate evidence

  • Exact full-diff threat scan: CLEAN. REST was exhausted, so the scanner used the freshly fetched complete diff bound to this head/base.
  • Focused gate: 46 passed, 3 environment skips at the full-gate head; the identical current-head freshen rerun also passed with clean head/tree/status.
  • Codex: SHIP ONLY WITH FIXES; reproduced the symlink escape and whitespace-only blank response.
  • Opus 4.8: SHIP with non-blocking nits; verified immutable mode ownership, parser/route structure, raw fidelity, and UI lifecycle, but did not catch the two reproduced blockers.
  • Fable 5.1: SHIP-WITH-UX-FIXES; requires the mobile disclosure fixes above.
  • Full suite at patch-identical a2946497…: 15,110 passed, 9 failed, 2 errors, 125 skipped. The 11 non-pass nodes are the known sandbox-topology set. The lightweight current-master freshen rule applies because the feature patch is identical and fix(runtime): report unverified Agent updates without automatic restart #7160 has no cron overlap. Intrinsic blockers were rerun at cc31f2ca… and make this head RED without relying on baseline attribution.

Please fix the three groups and re-push for a fresh current-master gate. Preserve @rodboev’s attribution. No merge, tag, deployment, contributor-branch push, or issue closure was performed by the gate certifier.

@nesquena-hermes nesquena-hermes added the gate-fail Gate found blocking issue(s); fix-spec in comment; awaiting fix/re-push label Sep 10, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate-fail Gate found blocking issue(s); fix-spec in comment; awaiting fix/re-push size:L Large PR (>10 files or >250 LOC) ux User experience / visual polish

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UX: show cron responses before raw prompt and execution context

2 participants