Skip to content

fix: report the real failure — harness diagnostics, Qoder stdout, and mode validation - #725

Open
jackeduzan-oss wants to merge 3 commits into
DietrichGebert:mainfrom
jackeduzan-oss:fix/report-real-failures
Open

fix: report the real failure — harness diagnostics, Qoder stdout, and mode validation#725
jackeduzan-oss wants to merge 3 commits into
DietrichGebert:mainfrom
jackeduzan-oss:fix/report-real-failures

Conversation

@jackeduzan-oss

Copy link
Copy Markdown

Four small correctness fixes found while reading the hooks and the correctness benchmark. Each is one commit, so they can be taken separately or partially. Every one is verified by a regression test, and npm test is green (87/86 pass + 1 skipped, 23, 3), as are check-rule-copies and check-versions.

The theme is the same in all four: a failure should say what actually went wrong, and a value should be validated where it enters.


1. The correctness harness reported its own crash instead of the failure

benchmarks/correctness.js, csv task:

except Exception as e:
    sys.stdout = _stdout      # restores the real stdout...
    pass
output = sys.stdout.getvalue()   # ...then calls getvalue() on it

Any exception in the scored snippet produced:

AttributeError: '_io.TextIOWrapper' object has no attribute 'getvalue'

So a missing pandas and a genuinely wrong model answer both surfaced as the same inscrutable harness traceback. The buffer now has its own name and stdout is restored in finally, and the exception the snippet raised is reported.

exec() had the mirror problem: it kept only stderr, while every harness prints its FAIL: output was ... line to stdout and exits 1. That diagnostic was unreachable, so failures read as a bare Command failed: python3 /tmp/.... It now carries a message field preferring stderr then stdout. stderr is still on the result object for any other caller.

Before / after on the same input:

reason: Command failed: python3 /tmp/ponytail-bench-...py
reason: FAIL: output was: '' (raised ModuleNotFoundError: No module named 'pandas')

The pandas case is now skipped when pandas is absent — the subject under test is the checker, not pandas, and npm test went red locally on any machine without it. CI installs pandas, so the case still executes there. A new test proves the checker itself is not pandas-dependent by passing a stdlib csv solution.

2. Qoder received two JSON objects from a bare /ponytail

hooks/ponytail-mode-tracker.js. The report-only branch wrote its status line, then the Qoder block wrote the ruleset — whose first line is that same PONYTAIL MODE ACTIVE — level: X string. stdout carried two concatenated JSON objects, which parses as neither:

{"hookSpecificOutput":{...,"additionalContext":"PONYTAIL MODE ACTIVE — level: full"}}{"hookSpecificOutput":{...}}

The mode-switch branch below already guards with !isQoder for exactly this reason; report-only was missed. Same guard applied, and nothing is lost — the ruleset write already opens with the suppressed line.

3. PONYTAIL_DEFAULT_MODE=" lite " silently resolved to full

getDefaultMode() did its own toLowerCase() without trimming, while the normalizeMode() it ships beside trims and validates against RUNTIME_MODES. One stray space in a shell profile or a CI yaml gave you the built-in default with no error. Now reuses normalizeMode for both the env var and the config field — same validation, including the #377 rule that review can never be a default.

4. A corrupt flag file was reported as the active level

readMode() returned the file verbatim, so /ponytail announced level: banana while getPonytailInstructions had already normalized it back to full — the status line contradicted the ruleset actually in force. The OpenCode reader already calls normalizePersistedMode on the same value (.opencode/plugins/ponytail.mjs:35); this one now does too. Also drops the unused getConfigDir import on the line being touched.


Verified end to end after the fixes:

result
Qoder bare /ponytail one parseable JSON object
PONYTAIL_DEFAULT_MODE=" lite " lite
flag file containing banana reports level: full, the mode really in force
snippet that raises (raised ModuleNotFoundError: No module named 'pandas')

Happy to split this into separate PRs or drop any commit if you'd prefer them apart.

The csv harness restored sys.stdout inside its except branch and then
called sys.stdout.getvalue() on it. Any exception in the scored snippet
therefore surfaced as:

    AttributeError: '_io.TextIOWrapper' object has no attribute 'getvalue'

so a missing pandas and a genuinely broken model answer produced the same
inscrutable harness traceback. Hold the buffer in its own name, restore
stdout in finally, and report the exception the snippet actually raised.

exec() had the matching problem from the other side: it kept only stderr,
while every harness prints its "FAIL: output was ..." line to stdout and
exits 1. The one useful diagnostic was unreachable, leaving failures as a
bare "Command failed: python3 /tmp/...". It now carries a message field
preferring stderr then stdout; stderr stays on the result for any other
caller.

Also skips the pandas case when pandas is absent. The subject under test is
the checker, not pandas, and `npm test` went red on any machine without it.
CI installs pandas, so the case still executes there.
The report-only branch wrote its status line, and then the Qoder block
wrote the ruleset — whose first line is that same "PONYTAIL MODE ACTIVE —
level: X" string. Two concatenated JSON objects landed on stdout, which
parses as neither.

The mode-switch branch below already guards with !isQoder for exactly this
reason; report-only was missed. Same guard, and nothing is lost because the
ruleset write already opens with the line being suppressed.
Two inconsistencies with the normalizers this file ships beside.

getDefaultMode did its own toLowerCase() without trimming, while
normalizeMode() trims and validates against RUNTIME_MODES. So
PONYTAIL_DEFAULT_MODE=" lite " — one stray space in a shell profile or a CI
yaml — silently resolved to the built-in default instead of lite, with no
error. Reuse normalizeMode for both the env var and the config field.

readMode returned the flag file verbatim. A hand-edited or truncated file
made `/ponytail` announce "level: banana" while getPonytailInstructions had
already normalized it back to full — the status line contradicted the
ruleset actually in force. The OpenCode reader already calls
normalizePersistedMode on the same value; this one now does too.

Drops the unused getConfigDir import while touching that line.
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.

1 participant