feat(advisory): let a lane answer from what was recently found here - #2154
feat(advisory): let a lane answer from what was recently found here#2154cohemm wants to merge 2 commits into
Conversation
An advisory lane re-derived everything on every question -- grepping the roster or taking a measurement each round, including for questions a lane had answered an hour earlier. It can now read what has recently been found in the same project and investigate only what is not already there. The boundary is recency, not the session (RFC #2153). What these two lanes report changes on the system's clock -- a commit lands, data flows -- not on the clock of whoever is being interviewed, so a finding from another session about the same project is as good as one from this session. Scoping to the session guaranteed every session's first question missed, which is most of the reuse there is. No index and no new state. Completed fan-outs are already published into a project-local artifact store, so what has been found here recently is a listing of that directory. Nothing is opened to build it -- modified time decides -- and what reaches the lane is paths, never contents: inlining findings would grow the prompt with every round, would make this server pick which of them matter without having read the question, and would put child-authored text on the producing side. The window rolls rather than following the calendar. The RFC fixes "on the order of a day" and leaves the value open, with one property that is not tuning: a calendar boundary empties at midnight into exactly the state that means nothing has been found here, which is the failure this mechanism keeps producing. Both traps the RFC records are closed with tests that fail without their fix. Both composition roots wire through one factory, because a wiring that reaches only one of them is invisible to every test that builds its own request. And the producer takes the store's own resolved root rather than deriving the same path at a later moment, since a relative workspace and a change of process directory in between would aim the reader at a store the writer never used. data_context's contract said the session was the measurement's time envelope; that wording is superseded here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Q00
left a comment
There was a problem hiding this comment.
Draft review (exact HEAD f587a02): recent_finding_paths() follows filesystem links at both levels. Path.is_dir() follows a two-character shard symlink, glob("*.json") traverses it, and path.stat() follows a JSON symlink. Because the returned absolute paths are explicitly handed to a child with instructions to read them, a workspace-controlled .ouroboros/artifacts/aa symlink (or a symlinked JSON body) can escape the artifact store and expose unrelated local files to the advisory lane. Please enumerate without following symlinks, require regular non-symlink shard/files, and verify resolved candidates remain beneath the resolved artifact root; validating the content-addressed filename shape would further keep the listing limited to actual store bodies.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2154 |
| HEAD checked | f587a024f732f594e4444753374d4f0dad9a7415 |
| Request ID | req_1786884782_2122-retry-1 |
| Review record | 9949e635-3b0d-4225-941c-80672538a535 |
What Improved
- Adds recent project-local fan-out discovery for PM
code_contextanddata_contextlanes. - Uses a rolling 24-hour window and newest-first path ordering without opening artifact bodies.
- Centralizes fan-out composition so publication and advisory lookup share the same resolved artifact root.
- Preserves advisory behavior when no workspace or recent findings are available.
Issue Requirements
| Requirement | Status |
|---|---|
| Implement RFC #2153: reuse recent findings from another session for the same project | Partially met — recent paths are exposed, but the lookup accepts forged/untrusted files and non-eligible fan-out outputs. |
| Use recency rather than session identity, with a rolling approximately one-day window | Met — the implementation uses a rolling 24-hour window. |
Offer only code_context and data_context findings |
Not met — all shard JSON artifacts are offered, including lateral and other advisory outputs. |
| Pass paths rather than finding contents and avoid opening files while building the list | Partially met — the producer only reads metadata, but it does not validate the path targets or artifact identity. |
| Keep the store/index stateless and use the existing project-local artifact store | Partially met — the existing store is used, but the direct filesystem walk bypasses its integrity and boundary checks. |
| Wire both composition roots to the same resolved publication root | Met — the new shared factory and composition tests cover both roots and relative-workspace changes. |
| Preserve behavior when no workspace or no recent finding exists | Met — callers without a root still receive their lanes and no recent-findings section. |
| Do not carry the prior session’s repository roster as finding metadata | Met — the prompt explicitly warns that findings may come from other sessions and retains the current roster boundary. |
Remove the obsolete observed_at data-context field |
Met — the changed schema no longer defines or requires it. |
Prior Findings Status
No prior ouroboros-agent[bot] review rounds were recorded for this PR; no previous concerns to maintain, modify, or withdraw.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/mcp/tools/recent_findings.py:64 | BLOCKING | The discovery walk trusts every *.json beneath any two-character directory and follows symlinks through is_dir(), glob(), and stat(). It does not require a regular, content-addressed artifact or verify that the resolved file remains inside the project artifact root. A workspace-controlled aa/forged.json symlink can therefore cause an absolute path outside the project to be handed to the advisory child, and a forged local JSON file can be presented as a completed finding. This bypasses the artifact store’s existing project-boundary, no-symlink, and integrity protections. Filter to validated content-addressed regular files and reject symlinks or otherwise route reads through the store’s integrity-checked fetch path. |
| 2 | src/ouroboros/mcp/tools/recent_findings.py:64 | BLOCKING | The lookup is not restricted to eligible code_context/data_context findings. The shared artifact root contains terminal outputs for multiple fan-out kinds, including lateral persona panels and normal interview advisories with contrarian, simplifier, architecture, and web lanes; this function lists all shard JSON files without inspecting their kind or lane set. The prompt then tells the child that each file contains reusable answers and to “use what helps” (src/ouroboros/mcp/tools/question_advisory.py:328). Consequently, ineligible reasoning or answer drafts can be reused as persistent system evidence, contrary to the PR’s explicit “two lanes are eligible and no others” boundary. The publication/lookup contract needs a lane- or artifact-kind-specific filtering mechanism before paths are offered. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.|
Test Coverage Notes
tests/unit/mcp/tools/test_recent_findings.pyandtests/unit/mcp/test_fanout_composition_wiring.pypass; the PM advisory tests also pass (105 passed).- Ruff checks pass for all changed source and test files.
- The changed adapter test file has 232 passing tests but 11 unrelated failures because the optional
mcppackage is not installed in the snapshot environment. - New tests cover recency, empty roots, path-only prompt construction, bookkeeping directories, and composition-root equality, but do not cover symlink/file-integrity rejection or filtering artifacts by fan-out kind and eligible lane.
Design Notes
The shared composition factory is a good structural improvement and correctly fixes the double-path-resolution risk. The new discovery layer is not yet safe as an artifact boundary because it treats a directory listing as authoritative without validating artifact identity, provenance, or lane eligibility.
Design / Roadmap Gate
The changed path crosses publication, persistent project-local storage, prompt construction, and child filesystem access. The publication store already has stronger containment and integrity semantics, but recent_finding_paths() bypasses them and passes raw paths directly into a child prompt. Because all fan-out kinds share the same content-addressed body namespace, recency alone cannot establish that a file is an eligible finding. Both failures affect production correctness and trust boundaries, not merely test completeness.
Directional Notes
Review focus followed the current source boundaries rather than prior review history: artifact-store integrity, project-controlled filesystem inputs, shared fan-out persistence, and the distinction between reusable system facts and question-specific reasoning. The PR body’s explicit “two lanes are eligible and no others” constraint was treated as authoritative.
Test Coverage
tests/unit/mcp/tools/test_recent_findings.pyandtests/unit/mcp/test_fanout_composition_wiring.pypass; the PM advisory tests also pass (105 passed).- Ruff checks pass for all changed source and test files.
- The changed adapter test file has 232 passing tests but 11 unrelated failures because the optional
mcppackage is not installed in the snapshot environment. - New tests cover recency, empty roots, path-only prompt construction, bookkeeping directories, and composition-root equality, but do not cover symlink/file-integrity rejection or filtering artifacts by fan-out kind and eligible lane.
Merge Recommendation
Request changes. The composition and recency mechanics are promising, but the current implementation can expose outside-project symlink targets or forged files and can feed ineligible fan-out reasoning back into factual lanes. Add validated artifact filtering and focused regression tests before merge.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: f587a02
request_id: req_1786884782_2122-retry-1
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Review found the listing trusted for two things it cannot establish. Both are RFC violations rather than hardening asks, and both close on one test. A body is offered only if its name is the digest of its own bytes. Content addressing is the store's own naming rule, so this is not a new guard: a body someone dropped in by hand cannot pass without finding a preimage, and the claims such a body carries would otherwise reach the user through a prompt that says the code says so. The same test does not cover a symlink to another project's valid artifact -- it hashes correctly and carries an eligible lane, and only where it lives makes it not this project's. That is cross-project reuse, which the RFC rules out, so symlinked shards and bodies are skipped. The first version of this test linked to an arbitrary file and passed on the digest check alone, which made the guard look unnecessary; it now links to a genuine body elsewhere and fails without it. And eligibility is read per lane. Every fan-out kind shares one content addressed namespace, so recency cannot establish that a file is a finding at all -- persona panels and code investigations sit beside advisories. Within an advisory, an interview turn runs six lanes into one submission, and the RFC admits two: what the system does and what it measures. A body carrying none of those is not offered, a body carrying some is, and the boundary inside it is named in the prompt rather than cut server-side -- slicing per lane here would put this server back in the position of deciding what is relevant without having read the question. The list is closed at two lanes. A web lane is excluded rather than pending: the window was derived from how a repository drifts, and nothing about this project bounds how fast an external fact turns over. Reading bodies to establish this retires the property that the listing opened nothing. Metadata still decides recency, so nothing outside the window is read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both blockers fixed in d2c0fdd, and both were RFC violations rather than hardening asks — worth saying, because it changes what the fix has to satisfy. Ineligible findings (blocker 2). The RFC closes the eligible list at A body carrying no eligible lane is not offered. A body carrying some is offered, and the boundary inside it is named in the prompt rather than cut server-side: slicing per lane here would put this server back in the position of deciding what is relevant without having read the question, which is the thing handing over paths exists to avoid. The list is now closed rather than open — the RFC was revised to exclude a web lane outright rather than defer it, since the window was derived from how a repository drifts and nothing about this project bounds how fast an external fact turns over. Integrity and containment (blocker 1). A body is offered only when its name is the digest of its own bytes. That is the store's own naming rule rather than a new guard, and a planted body would have to find a preimage for the name it sits under. The digest does not cover the case that matters most, though. A symlink to another project's valid artifact hashes correctly and carries an eligible lane — only where it lives makes it not this project's, which is cross-project reuse and something the RFC rules out. Symlinked shards and bodies are skipped for that. Worth flagging how close that one came to being missed: the first version of the symlink test linked to an arbitrary file, passed on the digest check alone, and therefore stayed green with the symlink guard removed — a test that looked like it pinned a guard and did not. It now links to a genuine body elsewhere and fails without it. All four guards (digest, symlink, kind, lane) were checked the same way: removed, re-run, confirmed failing. One property retired. The PR claimed the listing opened nothing. Eligibility lives inside the body, so that is no longer true and the body has been corrected. Recency is still decided from metadata, so nothing outside the window is read.
|
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2154 |
| HEAD checked | d2c0fdde8206827cf9bfb5478961f734d2718f21 |
| Request ID | req_1786899741_2141 |
| Review record | 5a7b19f1-5441-43ef-a27e-c223ebc4a0b1 |
What Improved
- Adds project-scoped discovery of recent advisory artifacts and passes paths rather than embedding child-authored contents.
- Correctly excludes symlinks, unrelated fan-out kinds, and bodies without
code_contextordata_contextentries. - Centralizes PM fan-out store wiring so both composition roots use the store’s resolved artifact path.
- Updates the
data_contextschema rationale to match cross-session recency.
Issue Requirements
| Requirement | Status |
|---|---|
| Let eligible advisory lanes reuse findings recently produced in the same project across sessions | Partially met — PM lanes receive paths, but ouroboros_interview never receives the artifact root |
| Keep the parent-facing contracted answer unchanged | Met |
Restrict reusable entries to the closed code_context and data_context set |
Met |
| Exclude web and question-specific reasoning lanes from reusable evidence | Met |
| Use a rolling window of approximately one day | Not met — mutable deduplicated blob mtime is not the latest publication time |
| Send artifact paths rather than inline findings | Met |
| Offer only valid, contained store artifacts | Not met — correctly hash-named project-planted bodies are accepted |
| Preserve safe bounded artifact handling | Not met — candidate bodies are read without the store’s size bound |
| Avoid a new index, registry, or duplicated state | Met |
| Wire both composition roots to the store’s resolved address | Partially met — PM is wired in both roots, while the ordinary interview producer is omitted |
| Preserve lanes when no workspace/artifact root exists | Met |
Correct data_context time semantics after replacing session scope with recency |
Met |
Prior Findings Status
The prior ineligible-lane and cross-project symlink concerns are addressed by current kind/lane filtering and explicit symlink rejection. The integrity concern remains in modified form: tampering under a pre-existing filename is rejected, but an attacker can trivially choose arbitrary bytes and name the file by their digest. Current HEAD also exposes new independently verified blockers in interview wiring, publication-recency semantics, and bounded reads.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/mcp/tools/definitions.py:545 | BLOCKING | The feature is not wired into ouroboros_interview, despite that being one of the advisory surfaces described by the PR. Both composition roots construct InterviewHandler without an artifact root; its unchanged _attach_question_assist_requests() consequently calls attach_question_advisory() with the default findings_root=None. Only PMInterviewHandler receives submit.artifact_root, so ordinary interview lanes never see recent findings on start, answer, or resume. Thread the resolved root through InterviewHandler and all of its question attachment paths, with composition-level creation and resume tests. |
| 2 | src/ouroboros/mcp/tools/recent_findings.py:142 | BLOCKING | Blob st_mtime is not publication recency in a content-addressed store. When an identical result is freshly published under a new contract, the store reuses the existing blob without updating its timestamp, so the new finding remains stale and is omitted. A focused probe published the same body under a fresh second contract and recent_finding_paths() returned []. Conversely, project code can touch an old blob and extend it indefinitely. Derive recency from the store’s publication/reference timestamp rather than mutable deduplicated blob metadata. |
| 3 | src/ouroboros/mcp/tools/recent_findings.py:106 | BLOCKING | Hash/name equality does not establish that a body was published by Ouroboros. A project-controlled process can choose arbitrary advisory JSON, calculate its SHA-256 filename, and place it in the corresponding shard; the current implementation offers it as trusted recent evidence. A focused probe confirmed such a correctly named forged body is returned. This contradicts the PR’s stated containment contract and enables project content to inject fabricated findings into later child prompts. Validate membership through an authoritative publication record, or explicitly redesign the trust boundary rather than treating content addressing as provenance. |
| 4 | src/ouroboros/mcp/tools/recent_findings.py:103 | BLOCKING | The new raw directory reader uses unbounded read_bytes() on every fresh candidate, bypassing the artifact store’s 1 MiB bounded-read contract. A project can plant arbitrarily large correctly hash-named files—or many ineligible files—and force every interview question to read and hash them before reaching the 20-path output bound. Use the artifact store’s bounded, link-safe reader and bound candidate enumeration before materializing bodies. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | tests/unit/mcp/test_fanout_composition_wiring.py:37 | Medium | High | Rename or split the “producer” helper so tests assert both PMInterviewHandler and InterviewHandler receive the publication root; the current helper only searches for PM and allowed the missing interview wiring to pass. |
Non-blocking Suggestions
| 1 | src/ouroboros/orchestrator/capabilities/interview_schemas.py:508 | Documentation | Reflow the long sentence beginning “It also asked…” to keep the schema rationale readable. |
Test Coverage Notes
- Read the PR body, human response, prior review classification, unified diff, changed files, and affected composition, prompt, artifact-store, schema, and resume paths.
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/mcp/tools/test_recent_findings.py tests/unit/mcp/test_fanout_composition_wiring.py -q: 18 passed.- Targeted adapter test: 1 passed, 245 deselected.
- Broader advisory selection: 60 passed, 1288 deselected.
- Focused runtime probes independently reproduced acceptance of a correctly hash-named forged body, unbounded oversized-file reading, and rejection of an identical freshly republished artifact because the reused blob retained its stale
mtime. - Existing tests cover PM prompt construction but do not exercise the ordinary interview composition or deduplicated publication semantics.
Design Notes
The path-only reuse model and centralized resolved-root wiring are directionally sound. The implementation currently treats raw content-addressed blobs as both publication records and authenticated evidence, but content addressing provides deduplication and integrity—not provenance or publication time.
Design / Roadmap Gate
This change crosses persistent project-local storage, composition roots, interview creation/resume, and child prompt construction. The PM boundary now shares a resolved root with publication, but the ordinary interview boundary is disconnected. The lookup also bypasses the artifact store’s authoritative reference timestamps and bounded reader, so replay eligibility can be stale, attacker-extended, forged, or resource-unbounded. These failures violate the PR’s stated recency, containment, and cross-runtime contracts.
Directional Notes
Review focus followed the advisory memory’s cross-entrypoint consistency and durable semantic-state lenses. Those hints were used only to inspect ordinary interview versus PM creation/resume paths and artifact recency; every blocker above was independently verified from current source or focused runtime probes.
Test Coverage
- Read the PR body, human response, prior review classification, unified diff, changed files, and affected composition, prompt, artifact-store, schema, and resume paths.
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/mcp/tools/test_recent_findings.py tests/unit/mcp/test_fanout_composition_wiring.py -q: 18 passed.- Targeted adapter test: 1 passed, 245 deselected.
- Broader advisory selection: 60 passed, 1288 deselected.
- Focused runtime probes independently reproduced acceptance of a correctly hash-named forged body, unbounded oversized-file reading, and rejection of an identical freshly republished artifact because the reused blob retained its stale
mtime. - Existing tests cover PM prompt construction but do not exercise the ordinary interview composition or deduplicated publication semantics.
Merge Recommendation
Request changes. Wire the resolved artifact root through every InterviewHandler question path, determine recency from authoritative publication references, reject bodies not backed by valid publication authority, and preserve the artifact store’s bounded/link-safe read semantics. Add regression tests for ordinary interview start/resume, identical fresh republication, correctly hash-named planted bodies, and oversized candidates.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: d2c0fdd
request_id: req_1786899741_2141
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Implements RFC #2153.
Summary
An advisory lane re-derived everything on every question: it grepped the roster repositories or ran a measurement, every round, for every question, including questions a lane had already answered an hour earlier. This lets a lane read what has recently been found in the same project and answer from it, so it investigates only what is not already there.
What the parent session receives is unchanged — the same contracted answer, with no way to tell whether the child read code, ran a query, or opened a file. That indifference is the point: a lane's job is defined by what it returns, not by what it touched.
The decision this implements
The RFC's boundary is recency, not the session. What these two lanes report changes on the system's clock — a commit lands, data flows — not on the clock of whoever is being interviewed. So a finding from another session about the same project is as good as one from this session, and scoping to the session had a cost that only looked free: every session's first question was guaranteed to miss.
Two lanes are eligible and the list is closed.
code_contextanddata_contextreport on the system; a contrarian, an answer drafter and an architecture reviewer produce reasoning about one question, which is not a fact that keeps. A web lane is excluded rather than pending — the window below was derived from how a repository drifts, and nothing about this project bounds how fast an external fact turns over.Why a day, and why a window exists at all. Two forces pull against each other. The same person asks about the same subsystems all day, so findings repeat — that is the entire source of value, and it wants the window wide. But code and data keep moving, so every hour a finding is a slightly worse statement about now — that wants it narrow. A day is where they balance for these two lanes. Stated as a balance rather than a safety margin deliberately: if it were only "nothing goes stale in a day", any argument that things are stable would be an argument for widening it.
Why a stale finding is survivable. Both eligible lanes end at a person — a
code_contextfinding is shown for confirmation before it is recorded as an adopted fact, and adata_contextmeasurement is shown beside the question and never recorded at all. So what checks whether a reused finding still holds is the reader, not a rule, which is why no rule was added.Shape
The RFC deliberately left open how a lane reaches a finding. This takes the shortest path available: completed fan-outs are already published into a project-local artifact store, so "what has been found here recently" is a listing of that directory, handed to the lane as paths.
That means no index, no new state, and no registry involvement. Nothing has to be kept in step with the store, because the store is already organised the way a lane needs to read it — per project, newest last, expiring on its own schedule.
Three properties worth stating:
Paths, not findings. The producer sends file paths and never their contents. Inlining what was found would grow the prompt with every round and would make this server choose, without having read the question, which findings matter — the child has read the question, so the child chooses and pays for reading only what it chose. It also keeps the producing side free of anything a child wrote, which this fan-out owes independently of this feature. A test pins it.
A listing is not authority, so what it offers is checked. Every fan-out kind shares one content-addressed namespace, and the directory sits in a workspace this process does not own — so recency alone cannot establish that a file is a finding, or that it is one of the store's files at all. A body is offered only when its name is the digest of its own bytes, when it is not a symlink, and when it carries a lane this decision admits. Content addressing is the store's own naming rule, so the integrity test is not an invention; a planted body would have to find a preimage for the name it sits under.
Recency is still decided from metadata, so nothing outside the window is read. Inside it, bodies are read to establish those two things — what they say stays the child's to weigh and none of it travels back with the path.
A rolling window, not a calendar day. The RFC fixes "on the order of a day" and leaves the exact value to implementation, with one property that is not tuning: the window must not have an edge that silently empties mid-session. A calendar boundary empties at midnight into exactly the state that means "nothing has been found here" — the failure this mechanism keeps producing — so the window rolls.
The two traps recorded on the RFC
Both were found the hard way in earlier attempts, and each has a test that fails without its fix (verified by removing the fix and re-running).
A wiring that reaches only one composition root. The producer is built in two places, and the first attempt wired the workspace into one of them. Every lane-level test stayed green, because each builds its own request and so never travels a composition. Both roots now go through one factory, and a test walks both and compares by value.
One address derived twice. The store resolved the workspace when it was constructed; a producer deriving the same path when a question is asked would resolve it at a different moment, so a relative workspace plus a change of process directory in between aims the reader at a store the writer never used. The producer takes the store's own resolved root instead. A test changes directory between composition and reading.
What is deliberately absent
No bookkeeping about incompleteness — no proving a stored finding sufficient, no reporting what a reuse left unsettled, no marking an answer as reused. What the answer contracts guard is that an answer cannot lie about its sources, and those guards hold whatever the child read. Completeness was never guarded and does not start being guarded here.
Carried consequence
A finding may come from a session that chose different repositories. Evidence from outside the current roster is already rejected at submission, so the failure is loud rather than silent, but it costs a round — so the lane is told where it is deciding what to read, rather than discovering it when its answer is refused.
data_contextcarries noobserved_aton the reasoning that the session is the time envelope. That reasoning is superseded by the RFC and the contract's wording is corrected with it.Test plan
uv run pytest tests/unit tests/conformance -q— 20321 passed, 88 skippeduv run ruff check src/ tests/anduv run ruff format --check src/ tests/uv run mypy src/ouroboros/mcp/python3 scripts/check-module-size.py --baseline-ref origin/mainNew coverage: