Repository navigation
feat: let an analysis cite from an explicitly versioned corpus - #230
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".
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35fb033f17
ℹ️ 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 107 to 110 in 50a01d1
Normalize nested judge depth failures as protocol errors
When the citation judge returns a deeply nested inner JSON value and the configured output cap is high enough to carry it, json.loads() raises RecursionError, which bypasses this handler and is reported as the generic E2E CITATION SUPPORT FAIL error=RecursionError instead of the stable ModelProtocolError path used for malformed judge responses. Catch decoder depth failures alongside ValueError, as the separate answer-evaluation parser now does.
AGENTS.md reference: AGENTS.md:L47-L47
ℹ️ 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: 2263b3ba56
ℹ️ 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 107 to 110 in d3ef64e
Normalize citation parser depth failures
On supported CPython 3.11, if the provider's nested citation-judgement string contains roughly 1,000 nested arrays and the configured output cap accommodates it, this json.loads() raises RecursionError, which is not caught here. The malformed response therefore bypasses ModelProtocolError normalization and ends as the generic E2E CITATION SUPPORT FAIL error=RecursionError; catch depth failures alongside ValueError as the other model-response parser does.
AGENTS.md reference: AGENTS.md:L47-L47
ℹ️ 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: 393540190f
ℹ️ 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: 0e8f86f710
ℹ️ 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".
Map JSON recursion exhaustion to ModelProtocolError and remove a newly linked artifact only when its inode is still ours after failed readback.
1 participant
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.
Verification
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-fixturetomain, review its complete incremental diff, and retain Draft until its independent live/budget gates are satisfied.