Skip to content

feat: let an analysis cite from an explicitly versioned corpus - #230

Open
DavidHLP wants to merge 51 commits into
mainfrom
agent/u02-citation-fixture
Open

DavidHLP wants to merge 51 commits into
mainfrom
agent/u02-citation-fixture

Conversation

@DavidHLP

@DavidHLP DavidHLP commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Summary

Supports the DAV-45 citation acceptance path and the development-only answer evaluator. This PR does not complete DAV-53/DAV-58 live acceptance or enable business writes.

  • Keep the pinned corpus and retrieval baseline unchanged by default; add a manifest-gated, explicitly pinned corpus override for the citation acceptance entry point.
  • Validate permission/scope/material class, source position, document/chunk identity and content binding; reject unsupported declarations, symlinks/FIFOs, duplicate sources/content, oversized files and mid-run input substitution.
  • Filter required submission status before ranking/limiting. Judge only citations the answer actually emitted; distinguish deterministic existence/provenance from model-judged support/derivability.
  • Add answer generation and judging for the development split only. Withhold expected outcomes from answer generation, reject sealed splits and malformed/forged citations, retain behavior mismatches and retry individual transport failures with billed attempts counted.
  • Snapshot evaluation inputs, reserve artifacts before model calls, publish complete bytes without clobbering and bind verdict read-back to the reserved directory. Sanitize provider diagnostics and failure artifact names.
  • Preflight every case query against the retrieval limit before opening a model session. Bound generated answers and check worst-case judge prompts against the same prompt budget before any billed request.

Verification

  • Remote-dev, clean exact head 7e330f3f159a943fac1d69c317fabb486ad15a2a: 300 passed in 3.24s using env -u DEEPSEEK_API_KEY -u DEEPSEEK_MODEL uv run --locked pytest -q on the eight affected answer-evaluation, manifest, budget, model, E2E, sourced-analysis and retrieval test modules.
  • A separate run over the checked-in 20 development cases exercised prompt-budget preflight with DeepseekModel and a rejecting httpx.MockTransport: preflight_ok cases=20 prompt_cap=24000 model_calls=0. No provider request was sent.
  • The checked-in 20-case invocation documents DEEPSEEK_MAX_CALLS=120 for its retry envelope; the default remains 64.
  • Eight review findings have been fixed and answered; all associated review threads are resolved.
  • Hosted CI for current head 7e330f3f159a943fac1d69c317fabb486ad15a2a completed successfully: https://github.com/DavidHLP/UltiCode/actions/runs/37199733612.

Scope and limitations

The checked-in and currently accepted corpus is agent-authored synthetic material. A manifest declaration alone is not a license or user authorization. Model-judged support is not human review; an evaluation finishing is not proof every measured outcome passed. Holdouts remain sealed. No live model evaluation was run during this closeout: the shared USD1 period still has an unknown-usage pending request and remains fail-closed.

Stacked delivery

Merge this PR with a merge commit to preserve ancestry. After merge, retarget #231 from agent/u02-citation-fixture to main, review its complete incremental diff, and retain Draft until its independent live/budget gates are satisfied.

analyze_submission read the pinned sample corpus directly, so how many citations an
answer could emit was fixed by that corpus: a status-filtered retrieval keeps only
fragments carrying the submission status, and the pinned corpus holds exactly one
such fragment. The acceptance asks for three.

The corpus is now an optional parameter that defaults to the pinned one, so the
recorded evaluation baseline is untouched, and the three-citation behaviour is
covered on a separately versioned fixture of three self-authored status-bearing
sources: three distinct citations, all passing the integrity gate.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-04T12:40:25.797210Z 96bc859 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 044cb93083

ℹ️ 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".

Comment thread services/agent/src/sourced_analysis.py Outdated
The docstring said an authorised corpus passes its own, but nothing in
analyze_submission checks a supplied corpus against the manifest — that enforcement
lives in load_sample_corpus and only covers the default. The parameter is a test
seam, and the docstring now says so, naming who authorises a supplied corpus.

Adds the companion assertion for the other side of the seam: with no corpus passed,
the pinned baseline still emits exactly one citation, from the one status fragment
it holds.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2b2abf178b

ℹ️ 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".

Comment thread services/agent/src/sourced_analysis.py Outdated
Comment thread services/agent/src/sourced_analysis.py Outdated
… the limit

Two review findings on the corpus seam.

A supplied corpus was accepted as-is: check_citations compares each citation
against the same caller-provided objects, so a document claiming a licensed scope or
a nonexistent source path could reach the model marked verified. The seam now
requires the corpus to arrive with its own manifest, passes it through
assert_manifest_covers (content digests and declared fields bound to the exact
text), and refuses anything not declaring itself agent-authored synthetic — a test
seam may exercise the evidence path but may not launder a document into real or
licensed material.

The status filter ran after keyword_search had already ranked and truncated to
MAX_RESULTS, so higher-ranked documents without the status could consume every slot
and a status-bearing document that answered the question was dropped. keyword_search
takes an optional require_text that narrows the corpus before ranking and before the
limit; analyze_submission passes the normalized status, which is the same set the
old post-filter selected, now ordered correctly.

Regressions: no manifest refused, a manifest whose text was swapped after
authorization refused, a corpus claiming real material refused, and a corpus where
three fillers outrank the only status-bearing document still emits that citation.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a567718b58

ℹ️ 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".

Comment thread services/agent/src/sourced_analysis.py
Both checked-in loaders reject a document over MAX_SOURCE_CHARS, and the new
documents/manifest seam did not: a manifest-bound SourceDocument of any size passed,
and keyword_search copies that text whole into every emitted citation — past the
bound that keeps a single source from taking over the model prompt.

The supplied-corpus branch now refuses an empty or over-cap document before a
citation is built, and the regression is red-first (DID NOT RAISE before the check).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b380dd9d0f

ℹ️ 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".

Comment thread services/agent/src/sourced_analysis.py Outdated
… bindings

The seam accepted `manifest` as `tuple[object, ...]`, so a caller could assemble
ManifestEntry objects directly. assert_manifest_covers only checks document bindings
and content digests, so declarations load_manifest is responsible for — blank
permission or scope, an unsupported model_input_projection, `source_trust="trusted"`
— were never checked, and the citation still read `verified`.

The parameter is now a path parsed by load_manifest inside analyze_submission, which
applies those rules before the binding check. In-memory entries are not accepted at
all: the only way through is a manifest file that survives the same validation the
checked-in ones do.

Regressions: all four declarations refused through the seam, and in-memory entries
refused outright.
The three-citation behaviour only existed in a unit test: the DAV-45 entry point
called analyze_submission with no corpus, so it always used the pinned corpus, which
holds one status-bearing document, and always stopped at insufficient_citations.

The analysis core is now shared by two callers with different material policies:

- analyze_submission keeps its synthetic-only rule — it is the test seam, and a
  fixture may exercise the evidence path but never present itself as real material.
- analyze_authorized_submission is the acceptance path: the caller pins the permission
  and scope it accepts, and every manifest entry must declare exactly those, after the
  same parse, content binding and source-cap checks. The corpus cannot grant itself a
  policy; authorised material flows because the run pinned it, not because a file said
  so.

The entry point takes ULTICODE_CITATION_CORPUS_DIR plus
ULTICODE_CITATION_CORPUS_MANIFEST — both or neither, resolved before the first
request — loads declarations from the manifest, refuses a symlinked or missing entry,
a declaration outside the pinned policy, and a file over the source cap, and hands the
same corpus and manifest to the worksheet. The default with neither variable set is
the pinned, manifest-gated loader, unchanged.

Tests: partial configuration refused, a declared-but-missing file refused, an
unsupported declaration refused before any call, the override corpus actually being
analysed (three rows judged, no material-gap failure), authorised material accepted
under a pinned policy, a policy mismatch refused, and the unit seam still refusing
authorised material.
…to end

The policy tests exercised the analyzer directly, and the override test proved only
the synthetic fixture through main_sync — neither showed the whole path handling
DAV-58-shaped material. The corpus fixture now takes the material fields it writes
(kind, scope, permission), and the new test runs main_sync with a real-kind corpus
under monkeypatched pinned policy, asserting three judged rows and that every verdict
row carries an override chunk id and none carries a pinned-corpus one: the verdicts
describe the material this run loaded, not the default.
#229 landed publication and lock regressions in the same test file this branch
extends; both sides are kept — the acceptance override tests and the publication/lock
regressions — with the markers removed only. Suite after the merge: 477 passed / 1
skipped.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8bc1928abe

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/src/sourced_analysis.py
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py
Comment thread services/agent/e2e_citation_support_model.py
Six review findings on the merge commit, all reproduced first.

P1 duplicate physical sources: two manifest entries with distinct ids could resolve to
one file, so a single fragment counted as several citations and the three-citation
gate could pass on a repeat. Resolved paths are now tracked and a second entry over
the same file is refused.

P1 documentation: the two operator-facing variables, their both-or-neither rule, the
manifest policy pin and the layout rules are now written up in services/agent/README.md
with the full reason list, and invoked in docs/DEVELOPMENT.md next to the rest of the
citation-support runbook.

Provenance label: the metadata and evidence line hardcoded `agent-authored-synthetic`,
so an authorised corpus would have been recorded as synthetic. Both now carry the
pinned permission the run validated.

Source position: derived from the loaded text instead of copied from the manifest, and
a declared position that does not match the file is refused — a one-line document
declaring `lines 900-999` can no longer travel into a citation as a verified location.

Malformed manifests: load_manifest failures (missing, unreadable, invalid JSON,
validation) are normalized to `corpus_manifest_unusable`, so the smoke emits its
evidence line instead of a traceback.

Test seam: the synthetic rule now covers the manifest permission as well as the
document fields, since load_manifest accepts a synthetic document carrying a licensed
permission.

Five regressions, each red first (duplicate source fails with error=ManifestError
before the fix, position mismatch and the label pass the run they must refuse, the
malformed manifest raises instead of printing its reason, the seam admits the licensed
permission). Suite: 482 passed / 1 skipped.
…from

The reason list omitted corpus_root_unusable (a symlinked or non-directory root) and
corpus_empty (a manifest declaring nothing), and both command samples showed only
DEEPSEEK_MODEL — an operator following either would hit a reason the runbook never
mentioned, or stop at deepseek_api_key_required without knowing the key belongs in the
environment or the secret store rather than on the command line.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa73caa945

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/src/sourced_analysis.py
Comment thread services/agent/e2e_citation_support_model.py
Six findings on `fa73caa94`, each with a regression that fails first.

Hard links: two names for one inode produce different resolved paths, so distinct ids
over one physical file slipped past the duplicate check and could satisfy the
three-citation gate on a repeat. Identity is now the filesystem's — (st_dev, st_ino).

Line offsets: the range was derived from the stripped text, so leading blank lines
disappeared and content starting on line 3 claimed `lines 1-N`. It now spans the first
to last non-blank physical line of the raw file.

Read failures: an entry that is unreadable or not UTF-8 raised OSError/UnicodeError
instead of the documented reason; both now normalise to corpus_entry_unusable.

Gap label: the insufficient_citations line hardcoded the synthetic label while the
success line used the selected one, so a gap under an authorised policy blamed the
wrong corpus.

Synthetic scope: the seam pinned the permission but not the scope, and the worksheet
publishes scope as permission_scope — a fixture could declare licensed material there.
The seam now requires the exact checked-in synthetic scope value verbatim; a substring
test would accept arbitrary text that happens to contain the phrase.

Empty manifest: `[]` was normalised into corpus_manifest_unusable, making the
documented corpus_empty unreachable; it is classified before validation now.
The reason list now covers the hard-link duplicate (same inode under two names), the
unreadable and non-UTF-8 entries that map to corpus_entry_unusable, the raw-file line
range that spans first to last non-blank physical line, and the empty-list manifest
that keeps corpus_empty instead of being normalised into corpus_manifest_unusable.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d860ec9f5c

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
…escriptor

Two findings on the merge commit.

Byte-for-byte copies under separate filenames have separate inodes, so the identity
check let one fragment be counted three times through `copy.md` next to `status-1.md`.
Loaded text is now digested as it is read and a repeated digest is refused with
corpus_entry_duplicate_content.

The entry was checked with is_symlink/stat and then read by a separate open, so a
path swapped for a symlink in between was followed. The read now happens once, on an
O_NOFOLLOW descriptor, with type and identity taken from fstat of the bytes actually
read; a link is refused by the kernel as corpus_entry_escapes_root.

Regressions, both red first: three byte-identical sources (accepted before, refused
now) and a symlinked entry whose path check is blind to links (the old code followed
it and failed later on position, the new code refuses at open and leaves the target
untouched). Docs name both reasons and the single no-follow open.
The previous symlink target differed from the manifest-bound entry, so the old code
failed on position after following the link — proving a mismatch was caught, not that
read-through was blocked. The target is now byte-identical to the entry (same digest,
same position): removing only the O_NOFOLLOW flag makes the run succeed, and the
descriptor path refuses it as corpus_entry_escapes_root with the target untouched.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 433e49e036

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/src/sourced_analysis.py
Comment thread services/agent/e2e_citation_support_model.py Outdated
Five findings on the review pass, each with a regression that fails first.

Anchored reads: O_NOFOLLOW only guards the final component, so a corpus directory
replaced by a symlink after the root check was still followed. The root is now opened
once with O_DIRECTORY|O_NOFOLLOW and every entry with dir_fd, fstat identity taken
from the descriptor actually read, and ownership transferred to fdopen before the
read so a failed read is reported once instead of EBADF masking it.

Bounded reads must reach EOF: a 1200-character prefix plus trailing whitespace strips
back to exactly the cap, so an arbitrarily large suffix passed the size check and the
digest bound to a prefix.

One snapshot: the manifest is parsed once, and the entries, documents and pinned class
travel together through the analyzer, the worksheet and the verdict metadata, so
replacing the file mid-run cannot leave them describing different material. The
snapshot type carries the four pinned declarations — permission, scope, sample kind,
access scope — because pinning permission alone would let a synthetic document ride
under an authorised permission. Declaration rules moved into validate_entries and are
re-applied to snapshot entries, so a hand-assembled wrapper cannot skip what parsing
enforces.

Regressions: suffix past the bounded read, symlinked root with the path check blinded,
a read failure reporting corpus_entry_unusable once, a material-class mismatch, the
manifest replaced after preflight, and a forged snapshot carrying source_trust or
projection the loader would refuse. Suite: 496 passed / 1 skipped.
The preflight read the manifest twice — once to classify an empty list, then again
through load_manifest — so "read once, one snapshot" was not true of the file itself
and a replacement between the two reads could leave the classification and the
validated entries describing different files.

Parsing is now split from reading: `parse_manifest_text` takes text and does the
duplicate-key check, the list/empty classification and every declaration rule;
`load_manifest` is a thin read-then-parse wrapper; an empty list raises `ManifestEmpty`
so a caller can distinguish "declared nothing" from "will not parse". The preflight
reads the file once and feeds that text to the shared parser.

The declaration rules now live only in `validate_entries`, called by the parser for
parsed entries and by the analyzer for snapshot entries, so the two cannot drift —
the inline copies in load_manifest are gone.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28064e954d

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py
Comment thread services/agent/src/sourced_analysis.py
Comment thread services/agent/e2e_citation_support_model.py
Three review findings on the snapshot-consistency commit.

The preflight returned a ValidatedCorpus without ever binding entries to documents, so
a manifest whose content_digest or chunk_id disagreed with its own files survived
until analyze_authorized_submission — after the login and submission scan — and surfaced
as a generic ManifestError instead of the documented corpus reason. The binding now
runs at preflight and ManifestError normalises to corpus_entry_unbound.

Raising ULTICODE_CITATION_REQUIRED_ROWS above three could never be reached: retrieval
caps at MAX_RESULTS, so the run always reported insufficient_citations no matter the
material. A threshold above the retrieval limit is now refused with its own reason,
before any gap comparison.

A declared source_path containing a NUL makes os.open raise ValueError before a
descriptor exists; the handler only caught OSError, so the workflow emitted
error=ValueError. It is now corpus_entry_unusable like every other malformed entry.

Red-first: with the fixes disabled the three regressions fail on the old reason, on
`insufficient_citations emitted=3 required=4`, and on `error=ValueError`. Suite 499
passed / 1 skipped. Docs name both new reasons.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 183321b8ee

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_citation_support_model.py
JUDGE_CONTRACT asked for the two booleans directly, so a compliant model
emitted {"supports": ..., "derivable": ...} at the top level while
_parse_decision only accepts {"answer": "<string>"}. The first billed
call then failed with 'model decision schema was malformed', so the
real-model citation-support run never produced a verdict.

The suite could not catch it: every test replaces DeepseekModel with a
stub that hands back the inner string without the adapter parsing a
response, and one assertion pinned the old wording itself.

Contract now names the envelope, and two tests cover the agreement
between the prompt and the real parser.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42be9f3b2e

ℹ️ 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".

Comment thread services/agent/README.md Outdated

DavidHLP commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Addressed review 5374837009 in b635757d. CLAIM, QUOTE and SUBMISSION_FACTS now travel as separate values in one serialized INPUT_JSON object, with explicit instructions to ignore embedded directives and forged labels. The regression checks escaping, exact round-trip and failed-gate handling. This validates the input boundary, not general model injection immunity.

Verification specifically for b635757: CI run 36813716062 completed successfully; its Agent job recorded 585 passed / 1 skipped on merge a71c49b. The newer head 335f981 retains this fix, but its CI run 36814764818 is still running and the separate FIFO discussion remains open.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 335f981b05

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_answer_evaluation.py
Comment thread services/agent/e2e_citation_support_model.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ef57f792a

ℹ️ 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".

Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/src/corpus_manifest.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d150170bf

ℹ️ 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".

Comment thread services/agent/e2e_answer_evaluation.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbb6e70af2

ℹ️ 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".

Comment thread services/agent/e2e_answer_evaluation.py
Comment thread services/agent/e2e_citation_support_model.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 86d5e95162

ℹ️ 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".

Comment thread docs/DEVELOPMENT.md Outdated
Comment thread services/agent/e2e_answer_evaluation.py Outdated
Comment thread services/agent/e2e_citation_support_model.py Outdated
Comment thread services/agent/e2e_answer_evaluation.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e992b139e5

ℹ️ 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".

Comment thread services/agent/e2e_answer_evaluation.py
Comment thread services/agent/src/corpus_manifest.py
Comment thread services/agent/src/corpus_manifest.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e6c1917786

ℹ️ 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".

Comment thread services/agent/e2e_answer_evaluation.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e330f3f15

ℹ️ 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".

ensure_ascii=True,
)
prompt = f"{JUDGE_CONTRACT}\nINPUT_JSON {data}"
decision = await model.decide([{"role": "user", "content": prompt}])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preflight every citation prompt before billing

When an external corpus yields differently sized citation rows and DEEPSEEK_MAX_PROMPT_TOKENS accommodates the early prompts but not a later one, this loop bills the early model calls before decide() raises ModelBudgetExceeded on the oversized row, leaving no verdict artifact. The call-count preflight explicitly avoids this paid-partial-run outcome, but the prompt budget is only checked per iteration; construct all row prompts and call model.check_prompt_budget() on them before the first decide().

Useful? React with 👍 / 👎.

Comment on lines +191 to +192
os.link(temporary, target.name, src_dir_fd=directory, dst_dir_fd=directory,
follow_symlinks=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Verify the inode linked from the publication temporary

When another process can modify the configured artifact directory, it can unlink or rename this temporary after its descriptor is closed and replace it before this pathname-based os.link; the published destination then names the replacement even though _publish returns the inode recorded for the original file. The advisory lock does not constrain a non-cooperating process, the answer workflow performs no artifact readback, and the citation workflow does not read back its metadata sidecar, so either workflow can report success with foreign artifact content. Verify the linked destination against the written inode and expected bytes, or publish using an operation tied to the still-open descriptor.

Useful? React with 👍 / 👎.

Comment thread services/agent/e2e_answer_evaluation.py Outdated
Comment on lines +292 to +298
print(
f"OK answer_eval scope=development_only "
f"supported={summary['supported']} unsupported={summary['unsupported']} "
f"not_applicable={summary['not_applicable']} "
f"completed={summary['completed']} incomplete={summary['incomplete']} "
f"behavior_match={summary['behavior_match']} deferred={summary['deferred']} "
f"sealed=holdout,holdout2"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Emit the generated artifact path on success

When the documented command uses the default destination, _artifact_path() chooses a random answer-eval-<token>.json name, but this success output never identifies it. A caller therefore cannot deterministically associate the completed run with its machine-evidence artifact—especially when multiple runs share the state directory—without guessing from directory listings or timestamps; include a sanitized artifact path or basename in the success line, as the citation workflow does for its verdict file.

Useful? React with 👍 / 👎.

Comment thread docs/DEVELOPMENT.md Outdated
it must return explicit retrieved chunk IDs in `citations`, and judging checks only those citations.
Holdouts remain sealed. Export the existing `DEEPSEEK_API_KEY` before this opt-in call; the entry
requires `DEEPSEEK_MODEL`; the documented command below sets `DEEPSEEK_MAX_CALLS=120` for the
checked-in 20-case development split (two logical passes × three billed attempts per case; the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove the volatile case count from canonical docs

This records the current development-case count directly in the canonical guide, so adding or moving a case immediately makes the explanation and its derived 120-call claim stale. Replace the volatile count with a command or formula that derives the required cap from executable configuration rather than embedding the current dataset size.

AGENTS.md reference: AGENTS.md:L140-L140

Useful? React with 👍 / 👎.

Comment thread services/agent/README.md
counts. They do not print response bodies, cookie names or values, tokens, submission source,
usernames, roles, tool names, source text, or model answer content.

### Answer-level evaluation (development split only)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove the duplicated answer-evaluation runbook

This new README section repeats the answer-evaluation contract, command, retry budget, artifact behavior, and operational guidance that this change also adds to the canonical docs/DEVELOPMENT.md. Maintaining both copies creates competing sources of truth that can drift whenever the runner changes; keep the runbook in the canonical core document and replace this section with a focused link.

AGENTS.md reference: AGENTS.md:L143-L143

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6301f765ea

ℹ️ 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".

# nested object it carries: `{"text": "a", "text": "b"}` must fail rather
# than read as silently last-write-wins.
parsed = json.loads(raw, object_pairs_hook=_reject_duplicate_keys)
except ValueError:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle nested JSON depth failures as protocol errors

On supported CPython 3.11, a model response whose nested answer string contains roughly 1,000 nested arrays causes json.loads() to raise RecursionError, which this handler does not catch. Because the 4,000-token output budget can accommodate such a payload, a malformed provider response can bypass AnswerEvaluationError and produce the generic FAIL error=RecursionError instead of the stable FAIL reason=protocol result; catch decoder depth failures alongside ValueError.

Useful? React with 👍 / 👎.

Comment on lines +290 to +291
descriptor = os.open(lock.name, os.O_RDWR | os.O_CREAT | os.O_NOFOLLOW | os.O_NONBLOCK,
0o600, dir_fd=directory)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Verify temporary-file creation before billing

When the persistent .lock file remains from an earlier run and its parent directory later loses write permission, this os.open merely reopens the existing writable lock inode, so the claim succeeds even though no new directory entry can be created. Both evaluation workflows can then complete all billed model calls before _publish gets EACCES while creating its .part file; create and remove a same-shape probe under the anchored directory descriptor during the claim so an unusable destination fails before any request.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96bc859a09

ℹ️ 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".

Comment thread services/agent/README.md
Comment on lines +71 to +74
### Corpus override for the acceptance entry point

`e2e_citation_support_model.py` can analyse a corpus outside the repository instead of the
pinned sample, without editing either:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Replace the duplicate corpus runbook with a link

This new section duplicates the corpus-override command, validation contract, failure reasons, and artifact behavior already added to the canonical docs/DEVELOPMENT.md; changes to the runner now require maintaining two long copies and can leave operators following stale instructions. Keep the detailed runbook in the canonical guide and replace this section with a focused link, as was done for the answer-evaluation section above.

AGENTS.md reference: AGENTS.md:L141-L143

Useful? React with 👍 / 👎.

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