Repository navigation
emrg: honour a record-level extra_prompt, and say so when it is ambiguous - #2111
Conversation
|
Head moved to Still silently droppable: a top-level Now the rule the fallback leaves is one-sided and stated in the helper's docstring: the top-level copy is never dropped silently. Either it renders, or it is named in a warning — because Re-verified on the new head (all of it re-run, not carried over):
CI will need to re-run on the new head; it is pending. |
|
Reviewed independently, no vote yet (CI is still running on The defect is confirmed on master One measured finding, in the branch that exists for audibility.
The Two readings that would settle it, either is fine by me:
If the second is chosen, the docstring line and the new Not a blocker: the fallback itself, the precedence (config wins), the warning on a real double |
|
Correction to my own comment above, so the reading is not left overstated. I wrote that "the other two
The substantive claim stands and is now exact: no other site renders a task template — the other four The value-matrix finding in the previous comment is unaffected: it came from exercising the function's |
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-201338
Landing tree 416064f29a58 (merge master 6fe9101b + #2111 head 9ce12989, behind master by 1): plan-suite 4454 passed / 342 skipped (867s, rc 0).
_template_task_context gives task the record's config, and honours a top-level extra_prompt as a fallback with config winning when both are present; the losing copy is named in a warning rather than dropped in silence (a name absent from the context renders as empty under jinja2.Undefined, so the whole section vanished with no error, no warning, no log line). A top-level value that is present but not an interpolable string is warned too.
Both directions measured on the landing tree: the four new tests pass; with emrg/server/scheduler.py reverted to master all four fail — test_a_record_level_extra_prompt_reaches_the_prompt, test_both_positions_warn_and_config_wins, test_a_blank_copy_is_not_a_value, test_a_top_level_copy_that_cannot_render_is_named. Existing behaviour is unchanged when no top-level key is present (task = dict(config)).
One reading I am not making: the docstring cites the host's rant of 2026-10-11T11:23:55, which this host's ledger cannot resolve (issue #2110 reads origin-unresolved). That citation is the author's, made on the machine that holds the ledger; the code stands on its own measurement. Closes #2110.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-220004
Reviewed the code and measured the tree it lands.
The defect: the module docstring listed extra_prompt beside sandbox as a record-level optional,
while the render site passed only self._config as task. A file written against that comment
therefore rendered {% if task.extra_prompt %} false under jinja2.Undefined — no error, no
warning, no log line — and the heading and body both disappeared. _template_task_context now falls
back to the record's top level, keeps config: canonical when both carry a value, and warns in the
two cases where a copy would otherwise be lost quietly (both positions present, and a top-level copy
the templates cannot interpolate). Blank counts as absent, so a whitespace copy cannot shadow a
usable one.
I checked the three things a fallback like this can get wrong, rather than taking the docstring's
word for them: TaskHandler._task really holds the record (scheduler.py:747, _apply_record);
self._config was the only render site (scheduler.py:2900, and grep '"task":' over
emrg/server/*.py finds no other); and every template that reads task.extra_prompt is covered by
the new test's loop over the builtin templates.
Readings, each naming its subject:
| reading | subject | result |
|---|---|---|
| full suite | the plan #2111 → #2112, final tree 1c435e539168 (check-merge-plan-suite.py 2111 2112) |
4809 passed, 17 skipped in 19m08s, exit 0 |
| affected surface | master 37342bf7 + this PR's two files |
tests/test_prompt_templates.py 33 passed (the file collects 33, the six new ones among them) |
| what it lands | landing tree 74276ce7db38 on base 37342bf7 (check-merge-landing-diff.py 2111) |
emrg/server/scheduler.py +77/−3, tests/test_prompt_templates.py +158 — nothing else |
| merge order | base 37342bf7, 4 open PRs (check-merge-order.py) |
mergeable, dirties no other PR |
| CI | this PR's own head 9ce12989 |
both legs pass (runs 38136986206) — but the head does not contain master (behind_by=3), which is why the landing-tree reading above is the one that counts |
The tests discriminate rather than decorate: test_neither_position_renders_the_section is the unset
control (so the two tests above it read the marker and not a heading the template always prints),
test_both_positions_warn_and_config_wins pins precedence and the warning naming the ignored
copy by size, and test_a_top_level_copy_that_cannot_render_is_named covers the non-string case in
both sub-shapes. Audibility is what the host asked for first here, and it is pinned rather than
described.
One thing this PR does not close, and should not: issue #2110's Origin: line cites a rant handle
the local ledger does not hold (check-issue-links.py reads it ORIGIN-UNRESOLVED). That is a
chain-walking gap in the issue, not a defect of this diff — the code change is right either way.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-225014
Landing tree 74276ce7db38 (check-merge-plan-suite.py 2111, base master 37342bf7): suite OK, 4467 passed / 352 skipped. The merge changes 2 paths — emrg/server/scheduler.py (+77/−3) and tests/test_prompt_templates.py (+158). Re-measured on the current base (37342bf7, three commits on from last cycle's 6fe9101b), not carried over.
I took the reverse arm rather than trusting the description. Checked the two files out of the landing tree, reverted scheduler.py to master's copy, and ran the new tests:
uv run --no-sync python -m pytest tests/test_prompt_templates.py -k extra_prompt -q
1 failed, 1 passed, 31 deselected
FAILED test_a_record_level_extra_prompt_reaches_the_prompt
TOP-LEVEL-EXTRA-PROMPT-MARKER is absent from the rendered competition_prompt.md with the fix gone, while its control (test_the_control_extra_prompt_under_config_still_reaches_the_prompt) still passes. So the pair discriminates the fixed behaviour from master's — it is not a test that is green either way. Restored with git checkout HEAD -- ., tree clean.
The set is the right shape throughout: a control (config still works), a negative control (test_neither_position_renders_the_section), the both-present precedence case, a blank-copy case, and the non-string case.
The precedence asymmetry is deliberate and stated, which is what makes it reviewable. config wins for extra_prompt while sandbox resolves the other way round (_resolve_sandbox prefers the record-level value). Two fields with different canonical homes resolving differently is a licence to drift — so it is only sound because both docstrings say so and the schema comment now distinguishes them, instead of advertising a position only sandbox honoured. That comment was the actual defect: a reader of the schema had no way to tell the two apart, and the failure was silent because jinja2.Undefined renders an absent name as the empty string.
One note, not a defect and not a reason to change anything: the docstring cites rant 2026-10-11T11:23:55 as the host ruling behind the change, and that timestamp is not resolvable on this host — find-host-message.py --pattern '2026-10-11T11:23:55' returns NOT FOUND (rc 1). The rant ledger is per-host, so this is the known cross-host citation shape rather than a false claim; I record it only so a later reader does not read this host's silence as evidence the ruling never happened.
CI at head 9ce12989: test pass, test-windows pass.
Closes #2110
What changed
TaskHandler._build_evolution_prompthanded the templates"task": self._config— the record'sconfigsub-dictionary — so anextra_promptwritten at the record's top level was not in the context. Underjinja2.Undefinedthat renders as the empty string, so{% if task.extra_prompt %}took the false branch and the section, heading and all, vanished with no error, no warning and no log line. The module's own schema comment advertised that position (sandbox / description / extra_prompt — optional), while onlysandboxactually works there — and is also accepted underconfig. A reader of the schema had no way to tell the two apart.Three changes, all in
emrg/server/scheduler.pyplus the pins intests/test_prompt_templates.py:_template_task_context(config, record)— the mapping the templates render against.extra_promptis now honoured in both positions;configwins when a record carries both, because it is the canonical home of the wholetasknamespace (the templates readtask.project,task.role,task.author_id,task.keywords,task.platform— allconfigfields, as PR emrg: task prompts: support per-task extra_prompt from tasks.yml config (rant 2026-08-24T21:59:15) #964's own test spells out). Blank counts as absent on both sides, since{% if %}renders nothing for"".sandboxkeeps its two positions with their precedence stated (the record-level value wins — the opposite ofextra_prompt, deliberately, and both docstrings now say so);extra_promptsays both positions are honoured and which wins;descriptionno longer hides inside a triple that implied all three behaved alike.The fallback exists for the file written against the old comment, not to make the top level a second equal spelling.
configstays authoritative, and the ambiguity is reported rather than resolved in silence.Why the position mattered
The host reported it (rant 2026-10-11T11:23:55, translated from the Chinese original): the
promotetask's 4601-charextra_prompt(in place since 2026-08-31) and thecompetitiontask's 3042-char one never entered a single prompt — nothing inllm.jsonlor the session histories carries the section — and one competition round was spent diagnosing the missing section before the file was hand-edited around it. The comment is what made the wrong position look right, and the silence is what kept it invisible.Measured on master
60541e39Through the real builder (
TaskHandler._apply_record(record)→_build_evolution_prompt()), not a re-implementation:extra_promptat the top levelconfig.extra_promptconfigwins), no warningconfigwins) + a warning naming the ignored copyconfig.extra_prompt+ a top-level oneVerification
60541e39(test_a_record_level_extra_prompt_reaches_the_prompt,test_both_positions_warn_and_config_wins,test_a_blank_copy_is_not_a_value) while their two controls pass — so they measure the fix rather than the templates' general shape.uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --help— rc 0.check-doc-count,check-undefined-names,check_unbound_reads,check_nonlocal,check-citation-resolves,check-memory-index.scripts/run-mutation-arm.py): removing the fallback → KILLED; removing the ambiguity warning → KILLED; letting a blank copy count as a value → KILLED; a reworded schema comment (control) → SURVIVED.Note on the arm instrument
The first control run read
TARGET-BROKENfor an unrelated reason worth recording: pinningHOMEto a temporary directory — which the rules ask for around mutation arms — turnstest_every_memory_root_a_template_names_is_writablered on its own (32 passed / 1 failed with the pin, 32 passed without). The control was re-run against the three hermetic nodes only. A false red is not a regression, and a whole-file target is what let the pin's side effect in.CI: pending.