[girish1430]

What this PR delivers

Repository observation, end to end: gh/glab measurements in, derived routing state out — girish's original design and behavior, taken over and re-homed onto the table-repository architecture that landed in #10.

• Forge provider (providers/forge.py): gh/glab through ProcessRunner, strict reply validation, both of Michele's required corrections (below)
• Observation modules by role: models/observation.py (target, facts, derived state — keyed by repository_id), services/observation.py (measurement machinery: change metrics, ban breaker, observe_forge_facts), repositories/observation.py (the store)
• The composite read: RepoStateQuery.observed() derives exposure class, risk tier, freshness, and open critical/high counts
• Context door: Context.repository_observation(designations=…) wires the whole flow over one SqlTraustUnitOfWork
• 839 unit + 205 Postgres integration tests, coverage 93.5%

Michele's required corrections (from review of dfd123a)

1. GitLab omitted diffs are no longer complete measurements — collapsed / too_large flags (per-diff and response-level) now mark the comparison truncated (incomplete), never a complete zero-line measurement. Regression tests for all three flag shapes.
2. GitHub ahead_by is strictly validated — null / true / 1.5 raise SourceError instead of coercing to 0/1/1. Regression cases added to the malformed-payload table.

Her verified behaviors all survive: baseline-change rejection, stale-write dropping, network=False, quota-deferred reuse, and the recorded-reply parity fixtures.

The philosophy

The data model is the guide; nothing reads around it.

• girish's original branch read three legacy projections (findings.db repos/findings tables, portfolio-graph nodes). This version deleted all of that: targets resolve through repositories.find_by_url → repository_refs.list_for → assessments.latest; open counts read the finding_status view (its is_open is the one definition of open); inventory membership is the repository table itself.
• No core-invented tables (portfolio_forge_facts is gone): observation persistence is a protocol + in-memory store until contracts lands repository_observation — the one named gap below.
• One owner per capability: classification lives on PortfolioPolicy (thresholds are config); the composite read is RepoStateQuery; the service layer keeps domain naming (PortfolioService) while persistence mirrors the model's sections exactly.
• Review asks from the earlier rounds are addressed: no cross-domain constructor (OpenCounts is a consumed-where-defined port), boundary data is Dto, designation falls back to the filing's value when the config file has no match, unreadable inputs log rather than silently returning empty.

What activates it

The flow is live but reads only the contract tables — so it lights up as the data migration populates repository, repository_ref and assessment. A repo not yet in the model is simply not observable (by design: bootstrap targeting comes from inventory, not from scraping).

The one open dependency

repository_observation — an append-only measurements table (repository_id, observed_at, status, comparison metrics) in contracts. Until it lands, observations are in-memory only (correct within a run; re-observed across runs). When it lands, the store becomes a TableSpec + SqlTableRepository in one small file and the NEED marker clears. lines_reviewed (churn denominator) carries a NEED(artifacts) marker for the same reason.

Also riding along

• .agents/skills/commit-prep/ — a repo skill that runs changes through the gates and up as a PR in one trigger (drafted during this work; small and self-contained)

Verification

make lint-fix · make test (839) · make coverage (93.5%, floor 80) · make test-integration (205, Postgres) — all green. New contract-table tests cover latest-audit selection, never-audited targets, inventory membership, and the view's decision semantics on both dialects.

Tracker: XWING-2214

[chubirka]

Not acceptable as-is. I would request changes before approving. Reviewed commit dfd123a.

The design is sound—store GitHub/GitLab measurements, then derive current repository state—but two reproduced parsing defects can understate repository changes.

Required corrections

1. GitLab omitted diffs are treated as complete measurements

providers/forge.py:175–190

The parser ignores GitLab’s collapsed and too_large flags. GitLab documents these as indicating omitted diff content.

Replaying those responses produced:

status="ok", changed_lines=0, compare_truncated=false

That incorrectly presents unmeasured changes as a complete zero-line comparison.

Correction: account for these flags; fetch omitted content where possible or mark the comparison incomplete. Add regression coverage for both flags.

2. Invalid GitHub commit counts are silently coerced

providers/forge.py:166–172

int(payload.get("ahead_by") or 0) accepts malformed post---jq payloads:

Input ahead_by	Accepted count
null	0
true	1
1.5	1

Correction: validate the required response fields and types before constructing Comparison. Invalid measurements should raise SourceError, not become successful observations. Add regression tests for these cases.

Verification

• 330 local unit tests passed; Ruff lint and formatting passed.
• Reviewed PostgreSQL CI logs: 26 integration tests passed.
• Live smoke exercised Context.repository_observation() through the real GitHub CLI and SQLite persistence.
• Confirmed state recomputation after findings were resolved, without additional network calls; baseline-change rejection; persistence across reopening; distinct URL-less filings; and network=False.

The normal path works. The two parser corrections and regression tests are needed before LGTM.

[smith-xyz]

Really solid work — the design is sound (agree with @chubirka's assessment there), the forge commands all run through ProcessRunner, persistence goes through SqlUnitOfWork, and I ran a drift scan over your branch: zero new hand-wiring, exactly the sanctioned wrappers. The test coverage (malformed forge replies, stale-write rejection, baseline invalidation) is the right test list.

Merge plan from my side: #7 (symbol pass) lands first, then this rebases. Heads-up on what that rebase carries, so you can budget for it — details inline. Plus one design ask (the repo_state composition) that I want to fold in while you're in there anyway.

The two forge parser fixes @chubirka requested are required before merge from my side too — no need to re-litigate those here, just noting agreement.

[smith-xyz]

One design ask for the rebase: PortfolioService holds FindingUnitOfWork and DesignationRepository directly — two other domains' persistence. The AGENTS.md design rules landing in #7 draw this as: repositories belong to their domain, services compose across domains by calling that domain's service, and cross-domain composite reads get their own query object.

The good news: the shape you were reaching for already has a designated home. #7 defines RepoStateQuery (services/portfolio.py, stub marked replaces traust#58 observe_repo) which composes PortfolioService + AssessmentService + FindingService and owns repo_state() / list(). On rebase, moving your implementation into that shape keeps repo_state() from being a special case — it's the sanctioned composite-read path, and later it can become a direct projection without touching anything else.

None of this is "you did it wrong" — you wrote against main, before the rules existed. It's the natural shape when the rules aren't there yet.

[smith-xyz]

Rebase heads-up, not a change request: #7's SourceProvider protocol has owners(), list_org(), languages() and doesn't know compare/quota. Yours is additive so conceptually it's a union — but on rebase it's worth deciding together whether forge-comparison ops belong on SourceProvider itself or on a narrower port that CommandSourceProvider additionally implements. Also noting ChangedFile/Comparison/Quota are Model here; #7's boundary-data convention is Dto (tolerant, at the edge) — worth aligning when the files merge.

Two more rebase items so they don't surprise you: (1) #7 renames Provenance → Acquisition, ArtifactIndex → BindingIndex, and folds TransitionPolicy into Disposition — your branch uses all three old names; (2) tests/v1/services/test_assessment.py and test_portfolio.py are deleted in #7 (replaced by the test_stubs.py discovery harness), so your versions will need porting into the new test layout.

[chubirka]

Accuracy revalidation: existing changes remain required

Reviewed head: dfd123af822d6a10f80a33fb9a854a9eb3986701, compared with core main at 5be9803.

This head is unchanged since the earlier changes-requested review. The existing required corrections remain in the source:

• GitHub comparison parsing: providers/forge.py:166–172 still uses int(payload.get("ahead_by") or 0). The previously reported invalid-value coercion has not been corrected.
• GitLab omitted diffs: providers/forge.py:175–190 still does not account for the per-diff collapsed / too_large indicators. The earlier request to avoid presenting omitted content as a complete measurement remains applicable.
• Cross-domain composition: services/portfolio.py:114–129 still injects FindingUnitOfWork and DesignationRepository directly into PortfolioService. State assembly has not moved to the designated RepoStateQuery service-composition boundary.

Current-main integration

An isolated git merge-tree --write-tree check against main 5be9803 reports 20 conflicted paths, including the contracts pin, context/contracts modules, portfolio and assessment services, renamed/deleted model and repository modules, tests, and uv.lock.

The next step is to rebase and adapt this implementation to the current table/repository model and public interfaces, reconcile the contracts pin, and address the existing parser and composition requests before rerunning integration validation. Passing tests against the older branch baseline would not establish compatibility with current main.

Verification limits

The unchanged parser defects were checked in source against the existing review; their previously reported failures were not rerun merely to reconfirm them. No new unit or PostgreSQL validation is claimed for this PR. The original reported 330-unit/26-PostgreSQL results are not new results from this revalidation.

Recommendation: keep changes requested. No PR source changes were made during this review.

[smith-xyz]

@chubirka re-review requested — your two required corrections are in (strict ahead_by validation; GitLab collapsed/too_large marks the comparison incomplete), each with regression tests. Since your review, the branch was taken over and re-homed onto the table-repository architecture from #10: no more legacy projection reads (targets/audits/open counts now come from the contract tables and the finding_status view), and the flow is keyed by repository_id. Your verified behaviors survive — the recorded-reply parity fixtures are unchanged. The write-up above covers the rest.

[smith-xyz]

@chubirka your revalidation landed minutes before the takeover push — please re-review against the new head 36a2517 (force-pushed to this branch; your review was of dfd123a). Mapping your points to where each is resolved:

1. GitHub ahead_by coercion — fixed at providers/forge.py:172–173: isinstance(ahead, bool) or not isinstance(ahead, int) raises into the malformed-response path; null/true/1.5 are regression cases in tests/v1/providers/test_forge.py.
2. GitLab omitted diffs — fixed at providers/forge.py:189–197: per-diff collapsed/too_large and response-level too_large mark the comparison truncated (incomplete), never a complete zero-line measurement; three regression tests.
3. Cross-domain composition — PortfolioService no longer holds FindingUnitOfWork or DesignationRepository (zero references). State assembly lives on RepoStateQuery (services/portfolio.py:199) with a consumed-where-defined OpenCounts port; designations feed the query, not the service.
4. Current-main integration — 36a2517 is a single commit directly on main 5be9803: the 20 conflicted paths are resolved by construction (the legacy-projection reads you'd have found in the merge are deleted entirely — targets, audits and open counts now read the contract tables and the finding_status view, keyed by repository_id). All five CI checks pass on this head, including postgres.

Your recorded-reply parity fixtures and the behaviors you verified (baseline-change rejection, stale-write dropping, network=False, quota-deferred reuse) are unchanged in substance — the re-home moved where data comes from, not what the flow does.

[chubirka]

Recheck: not fixed yet

Reviewed current head 36a2517e97b6a8277e7df0732f09afbfbbd48a26.

The original GitHub ahead_by and GitLab omitted-diff parser bugs are fixed, and the previous merge conflicts are resolved. However, three reproduced defects remain:

1. P1 — The advertised Context entry point is missing. context/context.py:122–142: repository_observation() is defined inside _Observation, not Context. Calling it raises AttributeError: Context object has no attribute repository_observation. Move the method onto Context and test that public entry point end to end.

2. P1 — SQL count wiring breaks the unit-of-work lifecycle. context/context.py:142–159 accesses uow.open_counts before entering the UOW; invoking the existing door body with its intended Context receiver raises an attribute error for open_counts. Capturing an initialized adapter is insufficient: RepoStateQuery.observed():226–238 resolves the target through a service that opens and closes the shared UOW, then uses the retained count adapter. A real SQLite-backed observation write succeeded, but the composite read raised RepositoryError: SqlOpenCounts ... This Connection is closed. Fix the transaction/adapter lifetime and test SQL-backed observation plus state assembly together; the current service tests use a fixed counts double.

3. P2 — A failed audit can become the freshness baseline. services/assessment.py:114–129 chooses the newest secure-code assessment without filtering its outcome. With real contract rows for a completed audit on September 1 and a newer failed audit on October 20, the service selected the failed audit, returned has_audit=True, and used its commit/date as the baseline. Select eligible successful audit outcomes before choosing the latest baseline.

Verification

• Python 3.12 full unit suite: 839 passed, 206 deselected.
• Ruff lint and formatting: passed.
• Merge check against main 5be9803: clean.
• The public-entry-point and SQL failures above were reproduced in smoke checks using real SQLite contract tables.
• PostgreSQL and coverage were not rerun.

The passing suite does not currently establish the end-to-end claim. Recommendation: keep changes requested. No PR source changes were made during this recheck.

[smith-xyz]

All three reproduced defects are fixed in 5742d55:

1. Door on the wrong class — repository_observation is a Context method again (a splice had moved it onto the _Observation dataclass). Regression: a new end-to-end door test (tests/v1/context/test_observation.py) builds a real Context over the contract tables and drives observe → derive through it — this test could not pass against the broken head.
2. SQL count wiring — root-caused to the unit-of-work lifecycle, not the count query itself: SqlUnitOfWork is now re-entrant (nested with blocks share the outermost connection and transaction; the outer owner rolls back and closes), and the door opens one session unit of work so the service, the composite read and open_counts share a live connection. Regression: nested-sharing unit test + the door test exercising counts across calls.
3. Failed audit as baseline — observation_target now passes status=ASSESSMENT_COMPLETED to AssessmentRepository.latest (new filter param): a degraded or failed run is never read as clean, per the assessment DDL's own rule. Regressions: a newer failed run does not displace a completed baseline; a repo with only failed audits reads as never-audited.

Gates on this head: lint clean, 843 unit, 208 Postgres integration, coverage 93.7%.

[chubirka]

/lgtm