mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 08:23:59 +01:00
fix: pre-landing review fixes (day bound)
Rows without a parseable string ts share one undated bucket instead of each escaping the bound; a ts without a zone is read as UTC, so the day does not depend on the machine's zone; days_span counts the same UTC days; question_id and user_choice must be strings. Tests pin a zone west of UTC. CHANGELOG says contributing_events is recorded in the profile JSON and not displayed yet (#3436). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
10821a1c43
commit
ffdedba75b
4 files changed
+45
-23
No files matched your search
+3
-3
@@ -2,10 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
**/plan-tune's inferred profile no longer pins at 1.0 after a few reviews, and it now shows how many answers moved each dimension.** This closes the last two parts of #2623. One /plan-eng-review produces about five answers, and every answer added its full delta, so eight sessions of "fix now" took `architecture_care` to 0.998, as uninformative as the 0.5 it replaced. Now each UTC day moves a dimension at most as far as one strong answer, and eight days of the same answers read 0.808. The profile also records `contributing_events` per dimension, so a dimension with no evidence reads 0 instead of a bare 0.5.
|
||||
**/plan-tune's inferred profile no longer pins at 1.0 after a few reviews, and it now records how many answers moved each dimension.** This closes the last two parts of #2623. One /plan-eng-review produces about five answers, and every answer added its full delta, so eight sessions of "fix now" took `architecture_care` to 0.998, as uninformative as the 0.5 it replaced. Now each UTC day moves a dimension at most as far as one strong answer, and eight days of the same answers read 0.808. The profile JSON (`--profile`) also records `contributing_events` per dimension, so a dimension with no evidence reads 0 there instead of a bare 0.5; /plan-tune's display and `--check-mismatch` don't read it yet (#3436).
|
||||
|
||||
### Fixed
|
||||
- **`bin/gstack-developer-profile`**: `--derive` sums each UTC day's deltas per dimension (by the row's `ts`) and clamps the sum to the largest single delta in the signal map (±0.06 today). Answers within a day net out before the clamp, and a row without a parseable `ts` is its own bucket. The bound ignores `session_id` on purpose: the PostToolUse hook and the agent log different ids for one review, and an unsubstituted `SESSION_ID` placeholder would merge every review into one bound or leave each answer unbounded. Several reviews on one day count as one. `--trace` uses the same accumulation, so it lists every contributing answer, each clamped day ("day 2026-04-01: +0.250 capped to +0.060") and a `TOTAL` line whose value matches `--derive`.
|
||||
- **`bin/gstack-developer-profile`**: `--derive` sums each UTC day's deltas per dimension (by the row's `ts`) and clamps the sum to the largest single delta in the signal map (±0.06 today). Answers within a day net out before the clamp. A `ts` without a zone is read as UTC, and rows without a parseable string `ts` share one undated bucket, so a junk timestamp can't escape the bound. `diversity.days_span` counts the same UTC days. The bound ignores `session_id` on purpose: the PostToolUse hook and the agent log different ids for one review, and an unsubstituted `SESSION_ID` placeholder would merge every review into one bound or leave each answer unbounded. Several reviews on one day count as one. `--trace` uses the same accumulation, so it lists every contributing answer, each clamped day ("day 2026-04-01: +0.250 capped to +0.060") and a `TOTAL` line whose value matches `--derive`.
|
||||
- **`bin/gstack-developer-profile`**: a logged choice named after an `Object.prototype` member, such as `constructor`, or a log line that parses to a non-object such as `null`, no longer makes every later `--derive` and `--trace` fail; signal lookups use own properties only. `--trace` replaces control characters in the logged fields it prints, so a logged `ts` can't forge a `TOTAL` line.
|
||||
|
||||
### Added
|
||||
@@ -15,7 +15,7 @@
|
||||
- `docs/designs/PLAN_TUNING_V0.md` documents `signal_events`, `contributing_events` and the per-day bound in the profile schema, including why the bound ignores `session_id`.
|
||||
|
||||
### Tests
|
||||
- `test/gstack-developer-profile.test.ts`: `contributing_events` counts unique rows when one answer writes two dimensions, and is all zero for an answer that matches no signal. The bound equals the signal map's largest delta. Five answers on one day move a dimension by exactly the bound in both directions, whether they carry one `session_id`, five different ones, the literal `SESSION_ID` placeholder, or one review logged by both the hook and the agent. The day is the UTC day of `ts`. `--trace` prints the clamped days and a total that matches `--derive`, and replaces control characters. A `constructor` choice and a `null` line are ignored. Negative controls: the same answers on separate days, a second day after a capped first day, rows without a parseable `ts`, and an answer pair that nets out are not clamped. The original positive tests fail against the old bin; the day-bound, `constructor` and control-character tests fail against the per-session version; a one-sided clamp fails the bound tests.
|
||||
- `test/gstack-developer-profile.test.ts`: `contributing_events` counts unique rows when one answer writes two dimensions, and is all zero for an answer that matches no signal. The bound equals the signal map's largest delta. Five answers on one day move a dimension by exactly the bound in both directions, whether they carry one `session_id`, five different ones, the literal `SESSION_ID` placeholder, or one review logged by both the hook and the agent. The day is the UTC day of `ts` even when the machine's zone is west of UTC, and a `ts` without a zone is read as UTC. `--trace` prints the clamped days and a total that matches `--derive`, and replaces control characters. A `constructor` choice and a `null` line are ignored. Rows with an unparseable or non-string `ts` share one undated bucket. Negative controls: the same answers on separate days, a second day after a capped first day, and an answer pair that nets out are not clamped. The original positive tests fail against the old bin; the day-bound, `constructor` and control-character tests fail against the per-session version; a one-sided clamp fails the bound tests, and local-day bucketing fails the UTC tests.
|
||||
- The test helper `logQuestion` sets `GSTACK_QUESTION_LOG_NO_DERIVE=1`, so the background derive can't race a test's own `--derive` (from #2625).
|
||||
|
||||
### Gate integrity
|
||||
|
||||
@@ -415,7 +415,9 @@ do_gap() {
|
||||
# not its session_id: writers log different ids for one review (the
|
||||
# PostToolUse hook logs the host conversation, the agent the preamble run)
|
||||
# and an unsubstituted placeholder such as SESSION_ID would merge or split
|
||||
# reviews. A row without a parseable ts is its own bucket.
|
||||
# reviews. A ts without a zone is read as UTC, and every row without a
|
||||
# parseable string ts shares one undated bucket, so junk timestamps cannot
|
||||
# escape the bound. diversity.days_span counts the same days.
|
||||
# The body is a single-quoted bash string: its JS must not contain a single
|
||||
# quote (use double quotes or backticks), or the assignment ends early and
|
||||
# every subcommand fails to parse.
|
||||
@@ -439,30 +441,31 @@ function accumulateSignals(lines, QUESTIONS, SIGNAL_MAP, dims) {
|
||||
for (const line of lines) {
|
||||
let e;
|
||||
try { e = JSON.parse(line); } catch { continue; }
|
||||
if (!e || typeof e !== "object" || !e.question_id || !e.user_choice) continue;
|
||||
if (!e || typeof e !== "object" || typeof e.question_id !== "string" || typeof e.user_choice !== "string" || !e.question_id || !e.user_choice) continue;
|
||||
count++;
|
||||
skills.add(e.skill);
|
||||
qids.add(e.question_id);
|
||||
if (e.ts) days.add(String(e.ts).slice(0, 10));
|
||||
const iso = typeof e.ts === "string" && /^\d{4}-\d\d-\d\dT\d\d:\d\d(:\d\d(\.\d+)?)?$/.test(e.ts) ? e.ts + "Z" : e.ts;
|
||||
const when = typeof iso === "string" ? new Date(iso) : null;
|
||||
const day = when && !Number.isNaN(when.getTime()) ? when.toISOString().slice(0, 10) : null;
|
||||
if (day) days.add(day);
|
||||
const def = Object.hasOwn(QUESTIONS, e.question_id) ? QUESTIONS[e.question_id] : undefined;
|
||||
const byChoice = def && def.signal_key && Object.hasOwn(SIGNAL_MAP, def.signal_key) ? SIGNAL_MAP[def.signal_key] : {};
|
||||
const deltas = Object.hasOwn(byChoice, e.user_choice) ? byChoice[e.user_choice] : [];
|
||||
if (!Array.isArray(deltas) || deltas.length === 0) continue;
|
||||
signalEvents++;
|
||||
const when = e.ts ? new Date(e.ts) : null;
|
||||
const day = when && !Number.isNaN(when.getTime()) ? when.toISOString().slice(0, 10) : null;
|
||||
const key = day ? "day " + day : "row " + count;
|
||||
const bucket = buckets.get(key) || { day: day || "undated row " + count, sums: {} };
|
||||
buckets.set(key, bucket);
|
||||
const key = day || "undated";
|
||||
const sums = buckets.get(key) || {};
|
||||
buckets.set(key, sums);
|
||||
for (const d of deltas) {
|
||||
bucket.sums[d.dim] = (bucket.sums[d.dim] || 0) + d.delta;
|
||||
rows.push({ ts: e.ts, question_id: e.question_id, choice: e.user_choice, day: bucket.day, dim: d.dim, delta: d.delta });
|
||||
sums[d.dim] = (sums[d.dim] || 0) + d.delta;
|
||||
rows.push({ ts: e.ts, question_id: e.question_id, choice: e.user_choice, dim: d.dim, delta: d.delta });
|
||||
}
|
||||
for (const dim of new Set(deltas.map((d) => d.dim))) contributing[dim] = (contributing[dim] ?? 0) + 1;
|
||||
}
|
||||
const totals = Object.fromEntries(dims.map((d) => [d, 0]));
|
||||
const capped = [];
|
||||
for (const { day, sums } of buckets.values()) {
|
||||
for (const [day, sums] of buckets) {
|
||||
for (const [dim, sum] of Object.entries(sums)) {
|
||||
const bounded = Math.max(-cap, Math.min(cap, sum));
|
||||
if (Math.abs(sum - bounded) > 1e-9) capped.push({ day, dim, sum, bounded });
|
||||
|
||||
@@ -105,7 +105,7 @@ After weighing Codex's argument, we chose to roll back CEO EXPANSION and ship an
|
||||
}
|
||||
```
|
||||
|
||||
`signal_events` counts the logged answers that moved any dimension, and `contributing_events` counts, per dimension, the logged answers that moved it (an answer that writes two deltas to one dimension counts once), so a dimension with no evidence (0) is visible beside one whose evidence balances out; both print 0.5 (#2623). Each value is the sigmoid of its accumulated deltas, and each UTC day's sum per dimension (by the row's `ts`) is clamped to the largest single delta in the signal map (currently ±0.06), so one burst of answers can't pin a dimension; `--trace <dim>` lists every contributing answer, each clamped day and the total. The bound keys on the day, not on `session_id`: the PostToolUse hook and the agent log different ids for the same review (the host's conversation id and the preamble's run id), and an agent can leave the `SESSION_ID` placeholder unsubstituted, so no logged id reliably marks one review. The cost is that several reviews on one day count as one. `contributing_events` still counts an answer logged by both writers twice. A row without a parseable `ts` is its own bucket.
|
||||
`signal_events` counts the logged answers that moved any dimension, and `contributing_events` counts, per dimension, the logged answers that moved it (an answer that writes two deltas to one dimension counts once), so a dimension with no evidence (0) is visible beside one whose evidence balances out; both print 0.5 (#2623). Each value is the sigmoid of its accumulated deltas, and each UTC day's sum per dimension (by the row's `ts`) is clamped to the largest single delta in the signal map (currently ±0.06), so one burst of answers can't pin a dimension; `--trace <dim>` lists every contributing answer, each clamped day and the total. The bound keys on the day, not on `session_id`: the PostToolUse hook and the agent log different ids for the same review (the host's conversation id and the preamble's run id), and an agent can leave the `SESSION_ID` placeholder unsubstituted, so no logged id reliably marks one review. The cost is that several reviews on one day count as one. `contributing_events` still counts an answer logged by both writers twice. A `ts` without a zone is read as UTC, rows without a parseable string `ts` share one undated bucket, and `diversity.days_span` counts the same UTC days. Nothing displays `contributing_events` yet (#3436).
|
||||
|
||||
**Diversity check** (Codex #13): `inferred` is considered "enough data" only when `sample_size >= 20 AND skills_covered >= 3 AND question_ids_covered >= 8 AND days_span >= 7`. Below this, `/plan-tune profile` shows "not enough observed data yet" instead of a potentially-misleading inferred value.
|
||||
|
||||
|
||||
@@ -422,19 +422,38 @@ describe('gstack-developer-profile per-day bound', () => {
|
||||
expect(inferred().contributing_events.architecture_care).toBe(10);
|
||||
});
|
||||
|
||||
test('the day is the UTC day of ts', () => {
|
||||
const deriveWestOfUtc = () => {
|
||||
const res = spawnSync(BIN_DEV, ['--derive'], {
|
||||
env: { ...process.env, GSTACK_HOME: tmpHome, TZ: 'America/Los_Angeles' },
|
||||
encoding: 'utf-8',
|
||||
cwd: ROOT,
|
||||
timeout: 30_000,
|
||||
});
|
||||
expect(res.status).toBe(0);
|
||||
};
|
||||
const atTs = (ts: unknown) =>
|
||||
logQuestion({ skill: 'plan-eng-review', question_id: 'plan-eng-review-arch-finding', question_summary: 'arch?', user_choice: 'fix-now', ts });
|
||||
|
||||
test('the day is the UTC day of ts, not the local day', () => {
|
||||
arch(1, 'fix-now');
|
||||
logQuestion({ skill: 'plan-eng-review', question_id: 'plan-eng-review-arch-finding', question_summary: 'arch?', user_choice: 'fix-now', ts: '2026-04-01T20:00:00-08:00' });
|
||||
runDev('--derive');
|
||||
atTs('2026-04-01T20:00:00-08:00');
|
||||
deriveWestOfUtc();
|
||||
expect(inferred().values.architecture_care).toBe(value(2 * 0.05));
|
||||
});
|
||||
|
||||
test('rows without a parseable ts are each their own bucket', () => {
|
||||
for (let i = 0; i < 3; i++) {
|
||||
logQuestion({ skill: 'plan-eng-review', question_id: 'plan-eng-review-arch-finding', question_summary: 'arch?', user_choice: 'fix-now', ts: 'not-a-date' });
|
||||
}
|
||||
test('a ts without a zone is read as UTC', () => {
|
||||
atTs('2026-04-01T23:30:00');
|
||||
atTs('2026-04-02T00:30:00Z');
|
||||
deriveWestOfUtc();
|
||||
expect(inferred().values.architecture_care).toBe(value(2 * 0.05));
|
||||
expect((readProfile() as { inferred: { diversity: { days_span: number } } }).inferred.diversity.days_span).toBe(2);
|
||||
});
|
||||
|
||||
test('rows without a parseable string ts share one undated bucket', () => {
|
||||
for (const ts of ['not-a-date', 'TS', 1775000000]) atTs(ts);
|
||||
runDev('--derive');
|
||||
expect(inferred().values.architecture_care).toBe(value(3 * 0.05));
|
||||
expect(inferred().values.architecture_care).toBe(value(CAP));
|
||||
expect((readProfile() as { inferred: { diversity: { days_span: number } } }).inferred.diversity.days_span).toBe(0);
|
||||
});
|
||||
|
||||
test('answers on one day net out before the bound applies', () => {
|
||||
|
||||
Reference in new issue
Block a user