feat(space): gate self_nag retrospective on evidence-quality preflight (#919) - #2452
feat(space): gate self_nag retrospective on evidence-quality preflight (#919)#2452lsm wants to merge 29 commits into
Conversation
#919) Low-evidence self_nag ticks were forced through episode generation via confirmLowConfidence, spending a judge call + review task on thin, process-level evidence that produces a no-op episode. Now, for triggerKind === 'self_nag', when the preflight (already computed upstream) is low-confidence AND the selection carries no substantive task/artifact/metric signal (only status notes / empty traces), skip createFromEvidence + createReviewTask, append a lightweight 'automation_noop' note on the goal, and advance the cursor either way so the next tick is not stuck. Genuine evidence still produces an episode even at a low preflight. - Expose EvolutionEpisodeService.preflightEvidence (reuses buildEpisodeInput so the gate and episode creation never disagree). - Add 'automation_noop' SpaceGoalEventType + migration 185 (widens the space_goal_events.event_type CHECK; same rebuild pattern as M183). - Refactor advanceCursor to take spaceId so the no-op path can advance without a review task.
![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: 9f0c8ceeb9
ℹ️ 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".
…-gate-self-nag-retrospective-on-evidence # Conflicts: # packages/daemon/src/storage/schema/migrations.ts
Address review feedback on the no-op gate predicate: - Use the selection's manual-only ratio (counts.manualNotes === counts.total) instead of checking counts.taskResults/workflowArtifacts/metricSnapshots. The old predicate kept thin ticks producing episodes whenever the scope had ever recorded a metric (counts.metricSnapshots reflects the scope-wide metricSnapshotCount, not the current batch), and it skipped substantive friction traces like slow_tool_call/permission_block that resolve to task context but are not task_result/friction_digest kinds. The gate now skips only when the preflight is low AND every selected evidence row is a manual note — immune to stale scope metrics, and any non-manual evidence (task results, friction traces, artifacts, in-batch metrics) keeps its retrospective. Tests updated to lock both regressions in.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b3bba71dd
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Anthropic)
Model: glm-5.1 | Client: HyperNeo | Provider: Anthropic
Recommendation: REQUEST_CHANGES
PR gates low-evidence self_nag ticks out of the episode-judge call by recording a lightweight automation_noop note on the goal and advancing the cursor. The premise is sound, and the prior review's push from scope-wide-metric to selection-based gating (commit 6b3bba71d) was the right move — a stale listMetricSnapshots(scope.id) can no longer keep thin ticks producing no-op episodes. Migration 186 (CHECK-widen, same rebuild pattern as M183), the advanceCursor refactor, and the shared-type automation_noop addition are all compatibility-safe and correctly wired.
The block is the selection predicate the fix landed on. It now keys on counts.manualNotes === counts.total, but two real evidence shapes defeat the stated intent. Both are corroborated by tracing the scorer (packages/shared/src/evolution-preflight.ts) and were flagged by the prior review; I re-verified them against the current head.
P2 — Empty no_friction trace diagnostics escape the gate (judge still fires on genuinely empty traces).
createTraceDiagnosticEvidence stores these as kind: 'session' (evolution-scope-service.ts:680-695), including the explicitly-empty metadata.status === 'no_friction' case (evolution-trace-evidence-service.ts:159-163, exercised in forge-evidence-capture.test.ts:367-374). The scorer does not count kind: 'session' toward manualNotes (evolution-preflight.ts:84), so for a one-row batch of an empty no_friction diagnostic: counts = { total:1, manualNotes:0, ... }, score = 10 (NON_MANUAL only), level = 'low'. The gate evaluates 0 === 1 → false and does not skip — exactly the empty-trace case the PR says it should skip. This is reachable when maxEvidencePerEpisode splits a task result from its later no_friction session diagnostic across ticks.
P2 — Substantive manual notes are permanently skipped.
addManualNoteEvidence accepts arbitrary summary text (evolution-scope-service.ts:449-457). For a manual-only batch, NON_MANUAL_SCORE (+10) never fires (total === manualNotes), so the maximum attainable score is MAX_OUTCOME_SCORE = 25 (evolution-preflight.ts:64) — well under MEDIUM_CONFIDENCE_SCORE = 45 (evolution-preflight.ts:56). So a note like "PR #2452 merged, CI green, QA validated" (matches PR/merge/CI/QA outcome tokens → outcomes=4, capped at 25) lands at level: 'low' with manualNotes === total → skip fires and the note is permanently excluded from any episode. No manual-only batch can ever reach level !== 'low' under the current caps, so the gate cannot distinguish a substantive manual note from a thin status note. This contradicts the PR intent ("thin, process-level… only status notes").
Both gaps share a root cause: the predicate equates "all-manual selection" with "no evidence," and "any non-manual row" with "substantive." The fix is to gate on the substantive signal itself rather than kind — e.g. additionally require counts.outcomes === 0 for the manual-only path, and inspect trace-diagnostic metadata.status === 'no_friction' (or have the scorer surface an emptyTraces count) so an empty session diagnostic also skips. Concretely:
function shouldSkipSelfNagNoOp(preflight: EvidenceQualityPreflight): boolean {
if (preflight.level !== 'low') return false;
const { counts } = preflight;
if (counts.total === 0) return false;
const substantive = counts.taskResults + counts.workflowArtifacts + counts.metricSnapshots + counts.outcomes;
return counts.manualNotes + counts.emptyTraces === counts.total && substantive === 0;
}(emptyTraces would be surfaced by the scorer detecting kind:'session' && metadata.status==='no_friction'; the cheaper interim is to also skip when the selection is manual-notes + empty-traces only.)
P2 — Migration 186 test omits the assertions M183 set as precedent.
space_goal_events carries 3 FKs (spaces, space_goals, space_tasks; index.ts:473-475) and 3 indexes. M186 rebuilds the table and toggles PRAGMA foreign_keys (migrations.ts:8102-8117), but migration-186-goal-automation-noop-event.test.ts asserts neither FK survival (M183 asserts PRAGMA foreign_key_list) nor that the FK pragma is correctly restored in the finally. The code path is correct and identical to M183 — this is a coverage gap, not a bug — but given the table has live CASCADE FKs the assertion should be backfilled so a future regression in the rebuild is caught.
P3 — No-op atomicity is untested, and the note-write silently no-ops when unwired.
runWriteTransaction is transactional only when deps.db is passed (handler.ts:343-345); the handler tests build inline deps without db, so the atomic note-write + cursor-advance path is never exercised under a real transaction. Separately, recordSelfNagNoOpNote silently returns when goalEventRepo is unset (handler.ts:509) — production always wires it (rpc-handlers/index.ts:704), so this is defensive, but worth a one-line note in the deps docstring that an unwired repo means the no-op is recorded only by the cursor skipReason.
Everything else is clean: the runMigration185→runMigration186 export swap is safe (185 has no importer and still runs via runMigrations); advanceCursor's 3 call sites are all updated and the function is module-local; preflightEvidence and createFromEvidence share one pure buildEpisodeInput computation so they can't disagree; and the interpolated score/maxScore in the note are numeric (no injection, Preact auto-escapes on render). I'd like to see the two behavioral gaps and the M186 FK-survival assertion addressed before merge.
…-gate-self-nag-retrospective-on-evidence # Conflicts: # packages/daemon/src/storage/schema/migrations.ts
Address the second round of review feedback on the no-op gate. The manual-only predicate was kind-based, which mis-classified two cases: - A substantive manual note (PR/CI/merge outcome in the text) was skipped solely because every row was kind 'manual_note'. - An auto-generated session trace diagnostic with no friction (kind 'session') was never skipped, so empty traces still ran the judge. The gate now skips only when the preflight is low AND there is no substantive signal: no concrete outcomes (counts.outcomes === 0, recognized by the scorer in any row's text/metadata) AND every row is a thin kind (manual_note or session — the only kinds that can be empty/process-level). Substantive manual notes, task results, friction traces (slow_tool_call/permission_block/...), artifacts, errors, and in-batch metrics all keep their retrospective. Added regressions for both cases (substantive note -> episode; empty session diagnostic -> skip). Verified via 5-space-agent-other (1684) and 4-space-migrations-a/b (284/280) shards.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f92dc822de
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Anthropic)
Model: glm-5.1 | Client: HyperNeo | Provider: Anthropic
Recommendation: APPROVE
Re-review of f92dc822. All four prior threads are resolved, and the content-aware gate is the right design. Verdict is approve — the three items below are non-blocking coverage/observation notes (two carried from the prior round), not correctness defects.
What I verified
- Both prior P2 gaps are genuinely closed. Traced the scorer in
packages/shared/src/evolution-preflight.tsagainst the newshouldSkipSelfNagNoOp(goal-automation-execute.handler.ts:507-514):- Substantive manual note →
countConcreteOutcomesfinds PR/CI/merge tokens →counts.outcomes > 0→ second guard returnsfalse→ episode produced. ✓ - Empty
no_frictionsession diagnostic →metadata.status === 'no_friction'matches noOUTCOME_PATTERNStoken, andsession ∈ THIN_EVIDENCE_KINDS→outcomes === 0andevery(kind ∈ {manual_note, session})→ skip fires. ✓
- Substantive manual note →
- The kind allowlist is comprehensive. Every substantive
EvidenceKind(task, workflow_run, metric_snapshot, task_result, artifact, all error/runtime kinds, retry_loop, tool/test_failure, permission_block, slow_tool_call, conversation_friction, friction_digest, verification_triage) falls outsideTHIN_EVIDENCE_KINDS, so any batch containing them keeps its retrospective. Onlymanual_noteandsessioncan be skipped, and only with zero outcomes. - Impact/compat surface is clean.
advanceCursor's 3 call sites are all updated to the new(deps, payload, evidence, spaceId, episodeId|null, context?)shape with correct argument mapping; it's module-local with no external callers.preflightEvidenceis a real instance method sharingbuildEpisodeInputwithcreateFromEvidence, so the preflight and the judge can't disagree. M186 (#2447's message-delivery index) and M187 both run in sequence — no gap, no duplicate. - The four new regression tests (thin manual note → skip + note; substantive note w/ outcomes → episode; slow_tool_call → episode; empty session diagnostic → skip) set up the claimed evidence shapes and assert the right outcomes with non-trivial expectations.
CI note (not a code issue)
Daemon Unit Tests (5-space-agent-other) shows red, but it failed at step 3 "Setup Bun" — every other job in the same run (including 4-space-migrations-a/b) passed that step, and the test step itself was skipped. It's a single-runner provisioning flake; a re-run will clear it. Flagging only so the green-CI gate isn't read as a test failure.
Non-blocking observations
P2 — M187 test omits the FK-survival assertions the repo sets as precedent (carried from prior review). space_goal_events carries 3 FKs (spaces, space_goals, space_tasks) and M187 rebuilds the table with PRAGMA foreign_keys = OFF. The rebuild is correct — it regex-renames only the table name + CHECK, so the FK/CASCADE clauses survive — but migration-187-goal-automation-noop-event.test.ts neither seeds FKs on the old table nor asserts PRAGMA foreign_key_list('space_goal_events') or that PRAGMA foreign_keys is restored in the finally. M180, M181, and M183 all assert foreign_key_list after their rebuilds; M187 deviates on the table with the most FKs. A future regression that silently dropped the FKs would pass this suite. Coverage gap only — the migration code itself is faithful to M183.
P2 — Outcome-token breadth caps the gate's real-world no-op rate (design observation). OUTCOME_PATTERNS matches very common engineering words (\bcheck\b, \btests\b, \bdone\b, \bpassed\b, \berror\b, \bfailed\b). A thin process-level note like "checked in, tests still pending, nothing done yet" hits outcomes > 0 and so is never skipped — the judge call still fires. This is a property of the shared scorer, not introduced here, and the PR's contract ("skip iff outcomes === 0") is implemented faithfully. Erring toward keeping episodes is the safe direction (over-skipping would lose a real retrospective), so this is not a blocker — but it does mean the kind-based "thin" intent won't be fully realized for manual notes that use ordinary dev vocabulary. Worth a note next to shouldSkipSelfNagNoOp if the no-op rate matters operationally.
P3 — No-op path atomicity is untested (carried from prior review). The no-op branch wraps recordSelfNagNoOpNote + advanceCursor in runWriteTransaction, but that helper is only transactional when deps.db is passed (handler.ts:345-347), and no test supplies it. The code path is correct; the atomicity claim rests on reading alone.
Summary
The core logic, migration code, type/compat surface, and regressions are all correct and address every prior finding. Approving — the three items above are optional polish the author can take or leave; none block merge.
…eview) Address the third round of review feedback. The content-aware gate treated every `session` evidence row as potentially thin, but a genuine session conversation summary (no outcome words) would be wrongly skipped. Only auto-generated session trace diagnostics are empty/process-level — and those carry `metadata.traceDiagnostic === true` (set by createTraceDiagnosticEvidence). The thin check now uses a per-row predicate: manual_note is thin; session is thin only when `metadata.traceDiagnostic === true`; everything else is substantive. Genuine session summaries keep their retrospective. Added a regression (session summary without the marker -> episode) and updated the empty-diagnostic regression to carry the marker. Verified via the 5-space-agent-other shard (1685).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 920787b851
ℹ️ 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".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Anthropic)
Model: glm-5.1 | Client: HyperNeo | Provider: Anthropic
Recommendation: APPROVE
Re-review of 920787b85149 (the head that landed after my prior APPROVE at f92dc822). The new commit narrows the no-op gate's session branch to a per-row predicate (isThinEvidence), so only auto-generated trace diagnostics marked metadata.traceDiagnostic === true count as thin — genuine session conversation summaries are no longer wrongly skipped. This directly addresses the design concern from the prior round and is the correct fix. Verdict is approve: zero blocking findings.
Note: posting as COMMENT because GitHub rejects self-approval on an own-PR (author and reviewer share the
lsmidentity). The marker line above carries the verdict.
What I verified (fresh, against this head)
- The
traceDiagnosticmarker is reliable in production. Traced everysession-kind evidence creation path. The only site that emits asessionrow iscreateTraceDiagnosticEvidence(evolution-scope-service.ts:680), which unconditionally setsmetadata.traceDiagnostic = trueat line 691. Both callers route ano_friction/empty-trace result into it (attachTaskEvidence:341,captureTraceEvidenceForCompletedTask:626). There is no competing code path producingsession-kind evidence, so the gate's session-branch (handler.ts:499) fires exactly for the diagnostics the PR means to skip, and never for a real session summary. The original PR purpose (skip emptyno_frictiondiagnostics) is preserved. - The per-row predicate is faithful.
isThinEvidence(handler.ts:497-501) returnstrueonly formanual_note, orsessionwithtraceDiagnostic === true; everything else is substantive.shouldSkipSelfNagNoOp(handler.ts:513-520) keeps the three preconditions (low preflight, zero concrete outcomes, all-thin). No substantive batch can be wrongly skipped: anytask_result/ friction trace /workflow_run/artifact/error/metric_snapshot/ genuine-session row failsisThinEvidence, soevidence.every(isThinEvidence)is false. - No selection mismatch between preflight and the gate. The handler builds
evidence(handler.ts:131) and passesevidence.map(i => i.id)topreflightEvidence;preflightEvidencere-fetches by those exact IDs (evolution-episode-service.ts:225-228) andshouldSkipSelfNagNoOprunsisThinEvidenceover the sameevidencearray. Same rows for both checks. - Regression coverage for the new branches is correct and non-trivial. Five cases pin the per-row predicate: thin manual note → skip + no-op note + cursor advanced (
goal-automation-service.test.ts:2766); substantive manual note with PR/CI/merge outcome tokens → episode (:2920);slow_tool_callfriction trace at low preflight → episode (:2847); emptysessiondiagnostic withtraceDiagnostic:true→ skip (:2989); and the new genuine-session-summary case (no marker, no outcome words) → episode (:3065). Assertions check episode absence/presence, cursorskipReason, and theautomation_noopevent — not trivialexpect(true). - CI is fully green on this head. All daemon unit shards pass, including
5-space-agent-other(which was a single-runner Setup-Bun flake on the prior head) and4-space-migrations-a/b; lint/knip/format/type-check and the coverage quality gate pass.
Carried-forward non-blocking note (optional, not a merge blocker)
The OUTCOME_PATTERNS token list in the shared scorer (packages/shared/src/evolution-preflight.ts:66-73) matches ordinary dev vocabulary (tests, done, check, failed, build). A genuinely thin process-level manual note that happens to contain those words still yields counts.outcomes > 0 and so spends a judge call — the kind-based "thin" intent is only fully realized for notes whose text avoids that vocabulary. This is a property of the pre-existing shared scorer (not introduced by this PR), errs safely toward keeping retrospectives rather than over-skipping, and is the right default. A one-line note next to shouldSkipSelfNagNoOp documenting that the no-op rate is bounded by scorer vocabulary would help future readers who care about the operational skip rate. Take it or leave it.
Summary
The head that landed after my prior approval correctly tightens the gate to a marker-based per-row predicate, the marker is reliably produced in production, no substantive evidence is at risk of being skipped, regression coverage is solid, and CI is green. Approving.
…eview)
Address the fourth round of review feedback. The gate bypassed the no-op path
whenever the scorer's counts.outcomes was nonzero, but that count matches bare
keywords ("ci", "build", "tests", ...) without negation or completion — so
no-op status notes like "CI has not run yet" or "build still pending" still
spawned a judge call and review task.
The gate now looks for an affirmative outcome signal directly on the selection:
a concrete PR reference or a merge/pass/fail-style result verb NOT preceded by
a negator/pending marker within the same clause. Bare completion words
("done"/"completed"/"approved") are excluded — they describe process status,
not work artifacts — which also restores skipping for the original thin
episode evidence ("task done, no result"). Added a regression for
negated/pending keyword notes and verified the logic against the reviewer's
examples plus the original episode's note texts.
…-gate-self-nag-retrospective-on-evidence # Conflicts: # packages/daemon/src/storage/schema/migrations.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc6116566
ℹ️ 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: b56c42e5cb
ℹ️ 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".
Address the fifth round of review feedback on the self_nag no-op gate:
- Metadata scanning now reads only string-valued fields, not the serialized
object — key names like {"passed": false} or {"error": null} no longer count
as affirmative outcomes, while an affirmative string value (e.g. a note
field saying "PR #7 merged") still does.
- The negation prefix now stops at clause boundaries (comma, semicolon,
colon, newline) in addition to sentence enders, so "No errors; tests
passed" keeps its affirmative "passed" instead of being suppressed by the
earlier "No".
Added regressions for both (cross-clause note -> episode; non-affirmative
metadata keys -> skip) and re-verified the full matcher matrix (12/12,
including all prior rounds' examples and the original thin episode texts).
…eview)
Address the sixth round of review feedback. The affirmative-outcome whitelist
only recognized PR references and result verbs, so a manual note carrying a
concrete measured change ("Benchmark latency dropped from 800 ms to 200 ms",
"Conversion rose from 3% to 5%") matched nothing and was skipped as thin,
permanently dropping a user-authored result.
Added a quantitative signal: a measured value with a unit changing to another
measured value (with the same negation-prefix guard, so "no change: still
800 ms" stays thin). A bare measurement without a change is an observation,
not an outcome, and stays thin. Added a regression (quantified note ->
episode) and re-verified the full matcher matrix (16/16 across all six
review rounds' examples plus the original thin-episode texts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfbf33e813
ℹ️ 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".
…PR review) Address the seventh round of review feedback. Negation was only checked before each outcome match, so "PR #123 is still pending" matched the PR reference and passed the gate, spawning an unnecessary judge call. Added a suffix qualifier check with the same clause-scoping rules as the prefix: a pending/negated word after the match within the same clause ("is still pending", "passing tomorrow", "waiting on review") suppresses it, while benign suffixes ("merged yesterday", "passed after the retry") keep it affirmative. Added a regression (trailing-pending PR reference -> skip) and re-verified the full matcher matrix (17/17 across all seven rounds' examples plus the original thin-episode texts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d573205b7
ℹ️ 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".
… score (PR review)
Address the eighth round of review feedback (three comments):
- Iterate every quantitative match instead of stopping at the first: a negated
leading measurement ("no change from 800 ms to 800 ms") no longer hides a
genuine later result in the same note.
- Treat future/conditional modals ("will pass", "PR #123 will merge",
"should/expected to/planning to") as non-affirmative qualifiers, with the
same clause scoping — a completed result in a separate clause still counts.
- Drop the preflight-level gate: the selection-aware content test now decides.
Scope-wide metrics and loose keyword outcomes could inflate an all-thin
batch to medium and bypass the no-op path; the level check was the last
place a score inflated by non-selection signals overrode the content check.
Updated the two long-lived self_nag tests to use substantive evidence (their
intent is per-tick episodes / cursor racing, not evidence quality) and added
three regressions: prospective claims -> skip, inflated-medium thin batch ->
skip, negated-first + genuine-second measurement -> episode. Matcher matrix
re-verified 20/20 across all eight rounds' examples plus the original
thin-episode texts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67ae3525b9
ℹ️ 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".
…omes (PR review)
Address the ninth round of review feedback (two comments):
- The persisted automation_noop goal note now interpolates the actual
preflight level instead of hardcoding "low" — after the content-based gate,
a medium-preflight thin batch is an explicitly supported skip case and the
timeline must not contradict it. The note also describes the content-based
decision (no affirmative outcome, all rows thin).
- Quantitative matches now go through the same suffix qualifier check as
keyword outcomes, so a prospective target ("800 ms to 200 ms is planned")
no longer counts as an achieved result. The suffix window may cross commas
and semicolons (a qualifier in a following coordinate clause still applies)
but stops at sentence boundaries, and goal/target/aim nouns are treated as
non-affirmative qualifiers.
Added regressions: planned quantitative target -> skip, and the
medium-preflight note text asserting "preflight is medium". Matcher matrix
re-verified 19/19 across all nine rounds' examples plus the original
thin-episode texts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c0b4b88b5
ℹ️ 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".
Address the tenth round of review feedback. The suffix scan crossed commas
and semicolons indiscriminately, so a pending status in an independent
clause suppressed an unrelated completed outcome in the same note —
"Tests passed; deployment pending" and "Tests passed, no errors" were both
classified thin and their cursor permanently advanced.
The suffix qualifier now applies only when it continues the matched clause:
either directly within it ("is still pending", "is planned") or immediately
after a comma (", still pending validation"). A qualifier in a new
independent clause with its own subject does not apply. The quantitative
regex also captures the trailing unit so the suffix window starts at the
right boundary ("800 ms to 200 ms, still pending" now correctly suppresses).
Added a regression (independent-clause pending -> episode). Matcher matrix
re-verified 24/24 across all ten rounds' examples plus the original
thin-episode texts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42775615e5
ℹ️ 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".
… (PR review)
Address the eleventh round of review feedback (three comments):
- Free-form concrete outcomes stay substantive: a deploy/release verb
followed by a structural artifact reference (commit SHA, semantic version,
URL, issue ref) counts as an affirmative outcome, so "Deployed release
v2.4.1 from commit a1b2c3d" keeps its retrospective. Artifact tokens are
unambiguous pointers (unlike outcome keywords), so they do not need the
negation machinery beyond the prefix check; prospective mentions
("will deploy v2.4.1", "planned release v3.0.0") still skip.
- Conjunctions introducing a new subject ("Tests passed and deployment is
still pending") now end the suffix scope, same as punctuated clause
boundaries — the qualifier belongs to the new clause, not the match.
- The preflight is now computed lazily, only on the skip path for the audit
note. Substantive self_nag ticks no longer pay a duplicate
buildEpisodeInput (scope-sized evidence/task/run/metric loads) since the
skip decision stopped using the score.
Added regressions for all three (deploy artifact -> episode; conjunction
subject -> episode; preflight-call count 0 on a substantive tick). Matcher
matrix re-verified 31/31 across all eleven rounds' examples plus the
original thin-episode texts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fa1b270be
ℹ️ 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 failed |
…iew)
Address the twelfth round of review feedback. hasArtifactOutcome checked the
prefix (deploy/release verb, not negated) but never the text after the
artifact reference, so a prospective release ("Release v2.4.1 is planned",
"Built v2.4.1 is still pending") counted as completed work and spawned an
episode + review task.
Artifact matches now go through the same suffixHasQualifier check used for
keyword and quantitative outcomes. Added a regression (prospective artifact
reference -> skip). Matcher matrix re-verified (32/32 across all twelve
rounds' examples plus the original thin-episode texts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 349d7a49c3
ℹ️ 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".
Address the thirteenth round of review feedback. The prefix negation check crossed unpunctuated conjunctions, so in "No errors and tests passed" the "no" belonging to the errors clause also suppressed the conjoined "tests passed" outcome — a manual-only batch was skipped and its cursor advanced past a successful test result. Prefix qualification now runs through prefixHasQualifier: a negator or pending word only applies when it sits in the outcome's own clause, i.e. after the last coordinating conjunction in the prefix window — the mirror of the suffix-side conjunction guard. Added a regression (conjoined negation -> episode). Matcher matrix re-verified 34/34 across all thirteen rounds' examples plus the original thin-episode texts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e90da5e758
ℹ️ 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".
…ries (PR review)
Address the fourteenth round of review feedback (two comments):
- Coordinated predicates share their clause's modal. A conjunction is a
prefix boundary only when a new subject follows it; when an unambiguous
coordination verb follows ("Tests will run and pass", "CI will run and
report failures"), the earlier "will" still scopes the outcome, so the
note stays thin. Ambiguous noun/verb words ("tests", "pass") keep the
conjunction as a boundary.
- The new-clause conjunction pattern now accepts multiword noun-phrase
subjects ("and build pipeline is pending", "and the release pipeline was
failing"), so their pending qualifiers attach to the new clause instead of
suppressing a completed outcome.
Added regressions for both (coordinated future modal -> skip; multiword
subject -> episode). Matcher matrix re-verified 36/36 across all fourteen
rounds' examples plus the original thin-episode texts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6adf086c0f
ℹ️ 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".
… review)
A manual note is now thin only when it carries status/pending language
(or is empty): a qualitative note without outcome keywords - a diagnosis,
lesson, or decision - is substantive content that keeps its retrospective.
STATUS_LANGUAGE_RE captures the recognizable signature of process-level
notes ("waiting for X", "not done yet", "will do Y", "need to deploy").
Requirement modals (need to / must / has to / have to / required to) and
goal words (goal / target / aim) join the non-affirmative prefix set so
prospective artifact mentions like "Need to deploy v2.4.1" or "800 ms to
200 ms is the goal" do not count as affirmative outcomes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db0e821504
ℹ️ 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".
…ing verbs (PR review)
Three refinements to the self_nag no-op gate. A manual note is now thin
only when EVERY clause carries status language - clauses split on
sentence-final punctuation (a period only when followed by whitespace or
end, so "v2.4.1" stays whole) and a colon does not split, so a note
mixing a status clause with a root-cause diagnosis keeps its
retrospective. A shared modal now scopes coordinated verbs behind
intervening adverbs ("Tests will run and eventually pass") via an
INTERVENING_ADVERBS skip in prefixHasQualifier. And a conjunction plus a
new subject plus a status adjective counts as a new clause even when the
linking verb is omitted ("Tests passed but deployment pending"), so the
shorthand status no longer attaches to the completed outcome.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ad9b63cd
ℹ️ 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".
…iew)
Two refinements to the manual-note thinness test. A colon now splits a
clause when the label before it is itself status language - "Blocked:
root cause was lock contention" delimits a status preamble from the
diagnosis that follows, so the note stays substantive (a plain
elaboration colon like "Progress: waiting on CI" still reads as one
status clause). And thinness now scans only the summary: metadata values
are arbitrary provenance/identifiers ({"source": "agent"}), not note
content, so they can no longer keep a process-only note substantive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ade88636
ℹ️ 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".
"done", "completed", and "approved" join STATUS_LANGUAGE_RE. They are deliberately excluded from AFFIRMATIVE_OUTCOME_RE (process status, not work artifacts), but were also missing from the thin-note vocabulary, so a note saying only "Done" bypassed the gate and spent an episode judge call. "complete" alone stays out - as an adjective it appears in substantive notes like "complete rewrite of the parser".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33d6f38ec6
ℹ️ 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".
Two refinements to the thin-note vocabulary and the prefix qualifier.
A clause whose only status signal is a bare modal is no longer thin when
it reads as a recommendation or decision - first-person subject with a
modal ("we should"), modal plus an advisory code verb ("should avoid
global mutable caches"), or a rationale clause ("because teardown
races"). Hard pending markers and ship/deploy modals still decide
immediately, so "tests should pass" and "Must publish release v3.0.0"
stay thin. And an outcome directly after an adversative conjunction -
even behind adverbs ("Tests might pass but eventually failed") - now
starts its own contrasting clause instead of inheriting the earlier
qualifier, while plain "and" coordination ("Tests will run and pass")
still shares the modal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a3f8b2910
ℹ️ 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".
NEW_CLAUSE_CONJUNCTION_RE now accepts modal auxiliaries as the verb of the new clause: "Tests passed and deployment will start tomorrow" — "and deployment will" is an independent future clause, so its "will"/ "tomorrow" qualifiers scope only that clause and the completed "passed" outcome stays affirmative instead of being discarded with the cursor advance. Linking-verb and omitted-verb status boundaries are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3780889a9
ℹ️ 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".
… negated findings (PR review) Four refinements. Manual-note clauses now also split at a conjunction that introduces an independent clause with its own subject, so "Rollout is blocked but root cause was lock contention" keeps the diagnosis. goal/target/aim joined the non-affirmative prefix set, so "Target release v2.4.1" no longer counts the artifact reference as an achieved outcome. The new-clause boundary patterns require at least one non-auxiliary subject word - "PR #123 opened and is still pending" is an elliptical coordinated predicate, not an independent clause, so the PR reference stays pending. And bare no/never/not are restricted to status phrases ("no update", "no new signal", "never got to it", "not working", and negated outcome verbs like "tests have not passed") - negative findings ("No index covers the query", "The cache is not the bottleneck") are substantive diagnoses.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b97638511e
ℹ️ 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".
Manual-note clauses now also split before because/since/so that: the content after a causal conjunction explains why - a diagnosis or rationale - so "Rollout is blocked because the cache key ignores tenant IDs" keeps its retrospective for the cache-key diagnosis even though "blocked" is a hard status marker. Pure status notes without causal content are unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f41e627e31
ℹ️ 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".
…e, metadata URLs (PR review)
Four refinements. The bare completion words (done/completed/approved)
now count as status only at the end of a clause - "Completed migration
to tenant-scoped cache" describes completed work and is substantive,
while "Done" and "Migration completed" stay thin. A status-label colon
splits only when the suffix carries diagnostic content (a linking verb
or 4+ words), so "Pending: CI" and "TODO: update docs" stay thin while
"Blocked: root cause was lock contention" splits. "since" splits only
when causal - a temporal duration ("Pending since Friday") is part of
the status. And the affirmative-outcome scan reads only a manual note's
summary: an externalUrl identifier for a still-pending PR must not read
as a merged-PR outcome.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6448d68867
ℹ️ 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".
…ting (PR review)
Four refinements. A status-label colon now splits only when the suffix
has BOTH a linking verb and four or more words, so "Pending: CI is
running" and "TODO: update the migration docs" stay thin while
"Blocked: root cause was lock contention" still splits. "due to" joins
the causal conjunctions, preserving "Rollout is blocked due to the cache
key ignoring tenant IDs". Conditional prefixes (if/when/once/unless/
until) mark outcomes prospective - "If tests pass, rollout is scheduled
tomorrow" no longer counts "pass" as achieved. And await/awaiting joins
the pending-status vocabulary ("Awaiting CI").
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac74c99dce
ℹ️ 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".
…ce noise (PR review)
Four refinements. A conditional before a conjunction keeps scoping the
conditions coordinated after it, so "If lint passes and tests pass,
rollout is scheduled tomorrow" rejects the second "pass" as prospective.
"blocked by" with an article introduces the blocking cause and splits
after it - "Rollout is blocked by a deadlock in the tenant cache" keeps
the diagnosis - while bare dependency shorthand ("blocked by review")
stays status. running / in progress / underway / in flight join the
pending-status vocabulary ("CI is running", "Migration in progress").
And session rows marked metadata.traceDiagnostic are excluded from the
affirmative-outcome scan - a "Trace evidence capture failed" diagnostic
is capture infrastructure noise, not a project failure outcome.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741e70be0f
ℹ️ 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".
| * does not apply. | ||
| */ | ||
| const SUFFIX_QUALIFIER_RE = | ||
| /^(?:[^,;:!?.\n]{0,32}\b(?:not|never|no|hasn'?t|haven'?t|hadn'?t|didn'?t|doesn'?t|won'?t|isn'?t|aren'?t|wasn'?t|weren'?t|can'?t|cannot|without|yet|still|pending|wait(?:ing)?|queued|incomplete|unfinished|tomorrow|scheduled|planned|will|would|should|could|might|goal|target(?:ed)?|aim)\b|,\s*(?:still|yet|pending|wait(?:ing)?|incomplete|unfinished|planned|scheduled|queued|without)\b)/i; |
There was a problem hiding this comment.
When a process-only note includes an artifact reference, such as Release v2.4.1 is in progress, the release context and version make hasArtifactOutcome() return true because this suffix vocabulary omits running, in progress, underway, and in flight. That affirmative result bypasses the thin-status check and creates the episode and review task the new active-execution handling is intended to skip; include these pending phrases in the outcome qualifier checks.
Useful? React with 👍 / 👎.
| * with any of these markers is process status regardless of anything else. | ||
| */ | ||
| const PENDING_MARKER_RE = | ||
| /\b(?:waiting|wait|await(?:ing|s)?|running|in\s+progress|underway|in\s+flight|pending|queued|incomplete|unfinished|planned|scheduled|upcoming|todo|blocked|stalled|goal|target|aim|need(?:s|ed)? to|must|has to|have to|required to|not\s+(?:yet|done|started|run|runned|finished|working|expected|passed|passing|merged|merging|landed|landing|shipped|shipping|failed|failing|green|succeed(?:ed|ing)?|deployed|released|built|closed|fixed)|no\s+(?:update|updates|movement|progress|news|change|changes|blockers?|findings?|errors?|failures?|issues?|new\s+signal|signal|new\s+work)|never\s+(?:got|started|finished|ran|completed|happened|landed|shipped|merged)|hasn'?t|haven'?t|hadn'?t|didn'?t|doesn'?t|won'?t|isn'?t|aren'?t|wasn'?t|weren'?t|can'?t|cannot|later|soon|tomorrow|tbd|n\/a|(?:done|completed|approved)(?:\s+(?:this\s+tick|for\s+now|today|this\s+week|so\s+far))?\s*[.!]?$)\b/i; |
There was a problem hiding this comment.
When running is a gerund in a substantive manual note, such as Running tests revealed a tenant-cache race, this context-free alternative marks the entire clause as process status. Because revealed and race are not affirmative-outcome keywords, the self-nag records low_evidence_noop and permanently advances its cursor past the diagnosis; recognize status-shaped uses such as CI is running without treating every occurrence of running as decisive.
Useful? React with 👍 / 👎.
| const CAUSAL_CONJUNCTION_RE = | ||
| /\b(?:because|due\s+to|so that|since\s+(?!(?:monday|tuesday|wednesday|thursday|friday|saturday|sunday|yesterday|today|tomorrow|january|february|march|april|june|july|august|september|october|november|december|morning|noon|night|evening|last|this|next|ago)\b)|blocked\s+by\s+(?:an?\s+|the\s+))\b/gi; |
There was a problem hiding this comment.
When a process-only note uses a short dependency explanation such as Blocked due to CI, this unconditional due to boundary splits the text into Blocked and due to CI. The latter has no status marker, so isThinManualNote() reports the selection as substantive and the self-nag creates an episode and review task for ordinary blocked-status evidence; only split due to when its suffix contains an actual diagnosis rather than a dependency identifier.
Useful? React with 👍 / 👎.
| * clause. | ||
| */ | ||
| const NEW_CLAUSE_STATUS_RE = | ||
| /\b(?:and|but|while|whereas|although|though)\s+(?:the\s+|a\s+|an\s+)?(?:(?!is\s|are\s|was\s|were\s|will\s|would\s|should\s|could\s|might\s|must\s|can\s|may\s|has\s|have\s|had\s)\w+\s+){1,3}(?:still\s+|yet\s+)?(?:pending|blocked|queued|incomplete|unfinished|planned|scheduled|waiting|stalled)\b/i; |
There was a problem hiding this comment.
Fresh evidence after adding the subject requirement is the adverb-led elliptical form PR #123 opened but still pending: this pattern consumes still as its required subject word and then recognizes pending as a separate clause's status. suffixHasQualifier() consequently detaches the pending qualifier from the PR, accepts the reference as affirmative, and creates an episode and review task for unfinished work; exclude still and yet from the subject position or otherwise require a genuine noun phrase.
Useful? React with 👍 / 👎.
1 participant
Low-evidence
self_nagticks were forced through episode generation (confirmLowConfidence: true), spending a judge call + review task on thin, process-level evidence that produced a no-op episode.Now, for
self_nag, when the preflight is low-confidence AND the evidence carries no substantive task/artifact/metric signal (only status notes / empty traces), the handler skipscreateFromEvidence+ the review task, appends a lightweightautomation_noopnote on the goal, and still advances the cursor so the next tick is not stuck. Genuine evidence still produces an episode, even at a low preflight. AddsEvolutionEpisodeService.preflightEvidence, anautomation_noopgoal-event type (migration 185, same CHECK-widen pattern as M183), and alow_evidence_noopskip reason.