Conversation
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
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.
There was a problem hiding this comment.
💡 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".
… 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.
There was a problem hiding this comment.
💡 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".
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).
There was a problem hiding this comment.
💡 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".
… 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.
There was a problem hiding this comment.
💡 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".
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.
There was a problem hiding this comment.
💡 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".
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.
There was a problem hiding this comment.
💡 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".
…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.
There was a problem hiding this comment.
💡 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".
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.
There was a problem hiding this comment.
💡 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".
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.
There was a problem hiding this comment.
💡 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".
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.
There was a problem hiding this comment.
💡 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".
U02 acceptance #3 asks for citation support, task completion and observed behaviour per development case, but keyword_evaluation only records retrieval facts and carries the answer-level trio as DEFERRED. These tests state the contract for an evaluator that fills them, refuses a sealed split, and reports a behaviour mismatch rather than hiding it.
keyword_evaluation measures retrieval only, so citation_support and answer_completion are DEFERRED and observed_behavior is not_measured. This adds the answer pass and the judging pass that fill those columns for U02 acceptance #3. Both passes use the adapter's answer envelope, matching _parse_decision, and the evaluator refuses any split outside development so the two holdouts stay sealed. An empty retrieval marks citation support not_applicable rather than failed.
Opt-in real-model run that fills the answer-level columns for the 20 development cases and writes a run-scoped artifact recording that the scope is development_only with both holdouts sealed. The call budget is checked against the plan before the first billed call so a short run cannot read as a completed evaluation.
The first real run aborted on httpx.ReadTimeout: the adapter's 30s default is per request, and 40 sequential calls on a reasoning model will stall eventually. Retrying the batch would rebill every case that already succeeded, so the retry sits per call and only covers transport failures — a protocol failure is deterministic and is not retried.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46371f5d27
ℹ️ 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".
The answering model was asked to declare which behaviour it had just performed, and that declaration became observed_behavior — so the metric reported the model's self-description rather than a property of the response it returned. The answer pass now produces text only, the judge classifies it from that text, and a self-declared label is refused as an unexpected field. The artifact also pins what was judged: manifest digest, per-document versions, and the case-file digest, alongside the raw answers which stay out of stdout.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fae735f56e
ℹ️ 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".
The first development-split run failed on dev-03 with finish_reason=length: the answering model spent the 2000-token cap on reasoning tokens and returned an empty answer. Retrying would not help at temperature 0, so the default is now 4000. The entry also classified ModelProtocolError as its own reason line instead of the generic error= label, and points at DEEPSEEK_MAX_TOKENS when the failure is a truncation — a cap problem should not send the next reader to inspect the JSON shape.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caea09c19e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a77b4d1aad
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbc2a5e424
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27b2f81e1e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
UltiCode/services/agent/e2e_citation_support_model.py
Lines 795 to 796 in 9bce536
Isolate corpus quotes from citation-judge instructions
When an authorized corpus fragment contains prompt-like text or lines such as SUBMISSION_FACTS: ..., this interpolation places it directly in the judge's instruction syntax; the loader permits arbitrary UTF-8 corpus prose and explicitly treats it as untrusted. Unlike the answer-evaluation judge, this contract neither serializes the untrusted fields nor explicitly tells the judge to ignore score-changing directives inside them, so a crafted quote can steer supports/derivable to true and make an unsupported citation pass the acceptance gate. Encode the claim, quote, and facts as clearly delimited values and add a malicious-quote regression.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b635757d32
ℹ️ 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".
|
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. |
There was a problem hiding this comment.
💡 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".
| collapse would have wrongly made it. The document keeps its original text; only | ||
| this comparison uses the canonical form. | ||
| """ | ||
| return _TRAILING_HORIZONTAL.sub("", _CARRIAGE_RETURN.sub("\n", text)) |
There was a problem hiding this comment.
Fresh evidence beyond the CRLF/trailing-space variants is that canonically equivalent Unicode text remains byte-distinct here: for example, composed é and decomposed e plus U+0301 produce different hashes. By varying multiple such characters, three visually and semantically identical external documents can pass duplicate detection and satisfy the three-citation acceptance gate with one piece of evidence. Apply Unicode normalization such as NFC before hashing the canonical text.
Useful? React with 👍 / 👎.
| # Reserved before any billed call: an existing artifact must not be | ||
| # clobbered, and an unusable destination is a failed run, not something to | ||
| # discover after paying for a whole batch of judgements. | ||
| lock = _claim_artifact(artifact) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier destination-reservation fix is a configured ULTICODE_ANSWER_EVAL_RESULT that is a dangling symlink. The shared claim helper checks Path.exists(), which returns false for that occupied pathname, so this reservation succeeds and the full answer/judge batch can run; publication then fails when the exclusive hard link encounters the symlink. Use lstat()/lexists() during the preflight claim so this case fails before any billed requests.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def _read_external_manifest(path: Path) -> bytes: | ||
| descriptor = os.open(path, os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK) |
There was a problem hiding this comment.
Fresh evidence beyond the corpus-root ancestor fix is that O_NOFOLLOW here protects only the manifest's final path component. If ULTICODE_CITATION_CORPUS_MANIFEST contains an ancestor directory that is already a symlink or is replaced with one before this open, preflight reads and records the digest of a different declaration than the operator-selected path; manifest-owned provenance such as document IDs, versions, and permission claims then appears validated in the artifact. Walk the manifest path component-by-component with no-follow descriptors, or use an equivalent no-symlink resolution primitive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 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".
| os.replace(temporary, target) | ||
| created = False # the rename consumed it | ||
| info = os.fstat(stream.fileno()) | ||
| os.link(temporary, target, follow_symlinks=False) |
There was a problem hiding this comment.
When the configured destination has a parent that another process can rename or replace during the billed model calls, the reservation remains locked in the original directory, but this path-based os.link resolves the parent again. Replacing that parent with a symlink redirects both the temporary file and final artifact into an unreserved directory because follow_symlinks=False does not protect ancestor components; both evaluation workflows can then publish corpus quotes or raw answers to an unintended location and report success. Keep an anchored directory descriptor from the claim and publish relative to it, or verify the parent identity immediately before publication.
Useful? React with 👍 / 👎.
| except ValueError as error: | ||
| raise ManifestError(f"corpus manifest is not valid JSON: {error}") from None |
There was a problem hiding this comment.
When an external manifest contains deeply nested JSON, still well below the 1 MiB read limit, Python's decoder raises RecursionError rather than ValueError. This exception bypasses both ManifestError conversion here and the _CorpusSourceError handling in _corpus_override, so the documented corpus workflow emits the generic E2E CITATION SUPPORT FAIL error=RecursionError instead of the stable FAIL reason=corpus_manifest_unusable that automation expects for malformed manifests. Catch decoder depth failures and convert them to ManifestError as well.
Useful? React with 👍 / 👎.
1 participant
Supports DAV-45 acceptance #2 (at least three cited fragments). Complements #229 and touches no file it changes.
Problem
analyze_submissionread the pinned sample corpus directly, so how many citations an answer could emit was a property of that pinned corpus. A status-filtered retrieval keeps only fragments carrying the submission status, and the pinned corpus holds exactly one — which is why the live run reportsinsufficient_citations emitted=1 required=3.Change
The corpus becomes an optional parameter that defaults to the pinned one, so the recorded evaluation baseline is untouched (
keyword_cases.jsonexpectations and their tallies are unchanged — no expectation was adjusted to fit a result). 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.Growing the pinned corpus instead was tried and rejected: with five documents at top-3, two development cases (
dev-13,dev-15) lost required evidence, i.e. tuning the corpus and top-k after seeing the baseline fail. That is the behaviour DAV-45 forbids ("先写预期再执行，不按结果改答案"), so the pinned corpus stays at three documents and the material question stays with DAV-58 (authorized real material).Verification
./.venv/bin/python -m pytest -q→ 460 passed / 1 skipped (the pre-existingqdrant_clientskip).test_keyword_evaluation,test_pinned_sample_baseline_is_untouched) pass unmodified, i.e. the evaluation tallies are byte-identical to before.